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

test(git): advanced test cases for include/exclude filters #5472

Merged
merged 17 commits into from
Dec 4, 2023

Conversation

vvagaytsev
Copy link
Collaborator

@vvagaytsev vvagaytsev commented Nov 22, 2023

What this PR does / why we need it:
In this PR:

  • add new test cases for include/exclude filters
  • fix test setup to make sure that the desired Git scan mode is used in all places

Which issue(s) this PR fixes:

Special notes for your reviewer:

@vvagaytsev vvagaytsev force-pushed the fix/5317-excluded-files-changes-trigger-rebuild branch 5 times, most recently from 86724cf to a2a6237 Compare November 24, 2023 09:48
@vvagaytsev vvagaytsev changed the title fix(git): don't rebuild container module on excluded files modification fix(git): don't rebuild modules and actions on excluded files modification Nov 24, 2023
@vvagaytsev vvagaytsev force-pushed the fix/5317-excluded-files-changes-trigger-rebuild branch 6 times, most recently from 6cf5c57 to 03b8d33 Compare November 27, 2023 15:37
@vvagaytsev vvagaytsev force-pushed the fix/5317-excluded-files-changes-trigger-rebuild branch 12 times, most recently from e144b09 to 19f2003 Compare December 1, 2023 11:25
@vvagaytsev vvagaytsev changed the title fix(git): don't rebuild modules and actions on excluded files modification test(git): advanced test cases for include/exclude filters Dec 1, 2023
@vvagaytsev vvagaytsev force-pushed the fix/5317-excluded-files-changes-trigger-rebuild branch 3 times, most recently from d99ef0d to 8d6b780 Compare December 1, 2023 14:32
@vvagaytsev vvagaytsev requested review from eysi09 and a team December 1, 2023 14:34
@vvagaytsev vvagaytsev marked this pull request as ready for review December 1, 2023 14:34
To make sure include filter behaves equally when exclude is empty and undefined
When the filename matches the filter, but the file path does not.
This does not make much sense.
Hashes must be verified in the disk-based test projects.
This makes the results fo the failed tests more readable.
To test the function against both `VcsHandler` implementations:
 - `GitHandler`
 - `GitRepoHandler`
Make sure the same scan mode is used in all git handlers created while testing.
Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

Looks good to me so far! 👍 as far as I understand the portions of the code that need to change in PRs that land soon are marked using the // FIXME comments, right?

{
name: "when directory is included by name with globs",
// FIXME: shouldn't just '**/deepdir' work well too?
pathBuilder: (_subDirName: string, deepDirName) => join("**", deepDirName, "**", "*"),
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to use both path.posix.join and path.win32.join in the future for tests, to make sure that we are compatible with both backslashes and forward slashes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I can add it in the next PR.

@vvagaytsev
Copy link
Collaborator Author

Looks good to me so far! 👍 as far as I understand the portions of the code that need to change in PRs that land soon are marked using the // FIXME comments, right?

@stefreak, no, those might be related to a separate bug. In the next PR I'll add test cases and fix for #5317. Let's see if that change fix also fixes the FIXME comments.

@vvagaytsev vvagaytsev force-pushed the fix/5317-excluded-files-changes-trigger-rebuild branch from b4aacd4 to b1462fa Compare December 4, 2023 10:40
@vvagaytsev vvagaytsev added this pull request to the merge queue Dec 4, 2023
Merged via the queue into main with commit 59a63dd Dec 4, 2023
45 checks passed
@vvagaytsev vvagaytsev deleted the fix/5317-excluded-files-changes-trigger-rebuild branch December 4, 2023 11:23
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