Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename GH1866 test, fix invalid char test, fix equality assertion order #3509

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

HebaruSan
Copy link
Member

Problems

  • The ZipValid_ContainsFilenameWithBadChars_NoException test is failing on Windows:

    1) Failed : Tests.Core.Cache.ZipValid_ContainsFilenameWithBadChars_NoException
      Expected string length 128 but was 27. Strings differ at index 0.
      Expected: "Error in step EntryHeader for GameData/FlagPack/Flags/Weyland..."
      But was:  "Illegal characters in path."
      -----------^
       at Tests.Core.Cache.ZipValid_ContainsFilenameWithBadChars_NoException() in C:\Users\User\github\CKAN\Tests\Core\Cache.cs:line 202
    
  • The above error message has swapped the expected and actual values of the string

  • The GH1866 test does not have a meaningful name. Every time I see it, I wonder what it does. It's hard to maintain it correctly when we are guessing about its purpose.

Causes

  • I think ZipValid's error format has changed over time, and the test has an old value
  • This test and a few others have the parameters to Assert.AreEqual in the wrong order, with the actual value first and the expected value second

Changes

I might self-review this, since it's just tests and shouldn't affect users.

@HebaruSan HebaruSan added GUI Issues affecting the interactive GUI Pull request Windows Issues specific for Windows Tests Issues affecting the internal tests labels Jan 7, 2022
@HebaruSan HebaruSan requested a review from DasSkelett January 7, 2022 22:03
Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the name of the test was weird. On the upside, you still knew exactly where to look for more information ^^
But the links do the job even better.

@DasSkelett DasSkelett merged commit 7b20733 into KSP-CKAN:master Jan 19, 2022
@HebaruSan HebaruSan deleted the fix/tests-cleanup branch January 19, 2022 22:50
@techman83
Copy link
Member

As the one who likely named the test 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issues affecting the interactive GUI Tests Issues affecting the internal tests Windows Issues specific for Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants