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

TestCaseSource on RegistryTests #434

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Thaina
Copy link
Contributor

@Thaina Thaina commented Dec 7, 2024

This is not request for registry but propose change on the RegistryTests so each registry entry would be tested separately for each case and have explicit case entry for easier investigating and rerun

@bdovaz
Copy link
Collaborator

bdovaz commented Dec 7, 2024

@Thaina please merge master in your branch

@Thaina Thaina force-pushed the expand-with-testcasesource branch from 66c611f to 806ce83 Compare December 7, 2024 15:26
@Thaina
Copy link
Contributor Author

Thaina commented Dec 9, 2024

Already merged

Do you think are there any concern about change test like this?
Such as, I have remove Ensure_Do_Not_Exceed_The_Maximum_Number_Of_Allowed_Versions which only do Assert.Inconclusive into Warn.If

@bdovaz
Copy link
Collaborator

bdovaz commented Dec 9, 2024

Let me check it this week and let you know.

The test you mention is to know which packages have more versions because there are some like AWS that makes daily builds and if we don't raise the minimum version in the registry.json file from time to time, it ends up growing infinitely causing disk space or processing/initialization time problems.

@bdovaz
Copy link
Collaborator

bdovaz commented Dec 11, 2024

Can you resolve the conflicts? Thanks!

@Thaina Thaina force-pushed the expand-with-testcasesource branch from 806ce83 to 9d1e6ea Compare December 11, 2024 04:19
@Thaina
Copy link
Contributor Author

Thaina commented Dec 11, 2024

@bdovaz resolved

@Thaina Thaina force-pushed the expand-with-testcasesource branch from 9d1e6ea to ed73087 Compare December 11, 2024 06:36
@bdovaz
Copy link
Collaborator

bdovaz commented Dec 11, 2024

  • Restore the Ensure_Do_Not_Exceed_The_Maximum_Number_Of_Allowed_Versions test.
  • Could it be that in some file you have changed EOLs? Because I see more changes than it should.

@Thaina
Copy link
Contributor Author

Thaina commented Dec 11, 2024

@bdovaz

  • As I have mention, Ensure_Do_Not_Exceed_The_Maximum_Number_Of_Allowed_Versions just use Assert.Inconclusive So it should be suffice with just Warn.If
    var versions = dependencyPackageMetas.Where(v => versionRange!.Satisfies(v.Identity.Version)).ToArray();
    Warn.If(versions,Has.Length.GreaterThan(maxAllowedVersions));

Not only it was Ensure_Do_Not_Exceed_The_Maximum_Number_Of_Allowed_Versions redundantly load whole registry in exact same manner as Ensure_Min_Version_Is_Correct_Ignoring_Analyzers_And_Native_Libs, it also bunching all of the lib that not pass the test into one long array. I think it would be more beneficial to separate warning into each of the testcase

  • Have you use Hide WhiteSpaces already?
    image

@bdovaz
Copy link
Collaborator

bdovaz commented Dec 11, 2024

@bdovaz

  • As I have mention, Ensure_Do_Not_Exceed_The_Maximum_Number_Of_Allowed_Versions just use Assert.Inconclusive So it should be suffice with just Warn.If
    var versions = dependencyPackageMetas.Where(v => versionRange!.Satisfies(v.Identity.Version)).ToArray();
    Warn.If(versions,Has.Length.GreaterThan(maxAllowedVersions));

Not only it was Ensure_Do_Not_Exceed_The_Maximum_Number_Of_Allowed_Versions redundantly load whole registry in exact same manner as Ensure_Min_Version_Is_Correct_Ignoring_Analyzers_And_Native_Libs, it also bunching all of the lib that not pass the test into one long array. I think it would be more beneficial to separate warning into each of the testcase

  • Have you use Hide WhiteSpaces already?
    image

I agree with the test you mention.

Normally I don't touch that "Hide whitespace" checkbox because if I check it I don't see this kind of problems I mention, have you checked that you haven't changed any EOL? Because that's what it looks like.

@Thaina Thaina force-pushed the expand-with-testcasesource branch from ed73087 to 23d2e81 Compare December 11, 2024 11:40
@Thaina Thaina force-pushed the expand-with-testcasesource branch from 23d2e81 to 9a86758 Compare December 11, 2024 11:42
@Thaina
Copy link
Contributor Author

Thaina commented Dec 11, 2024

have you checked that you haven't changed any EOL?

I don't really sure, I use vscode and I don't know if it saved with difference EOL than the source or not and it also not report any change except 2 lines of code that it trimming whitespace automatically

@Thaina
Copy link
Contributor Author

Thaina commented Dec 13, 2024

@bdovaz If the EOL was changed can you just edit that to merge the pull?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants