Skip to content

More inputs invalidation test; fix for deletions #3987

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

Merged

Conversation

davidmorgan
Copy link
Contributor

For #3811.

More test coverage. I found another bug which is a regression from published behaviour, so the second commit fixes it.

The bug is about a change of input from present->deleted not triggering generation.

The problem is that the graph update code removes inputs if the input was removed, so by the time _buildShouldRun is reached there is no more record of the dependency. There must have been some code accounting for that case that got simplified away :) ... fix it by keeping the inputs for deleted files, changing the deleted nodes to missingSource nodes.

Copy link

github-actions bot commented Apr 29, 2025

PR Health

@davidmorgan davidmorgan force-pushed the more-inputs-invalidation-test branch from b4256a9 to 1bc72d3 Compare April 29, 2025 15:02
@davidmorgan davidmorgan force-pushed the more-inputs-invalidation-test branch from 1bc72d3 to cc39f18 Compare April 29, 2025 15:33
@davidmorgan davidmorgan force-pushed the more-inputs-invalidation-test branch from cc39f18 to 6a19676 Compare April 29, 2025 16:02
@davidmorgan davidmorgan changed the title More inputs invalidation test More inputs invalidation test; fix for deletions Apr 29, 2025
@davidmorgan davidmorgan requested a review from jensjoha April 29, 2025 16:28
@@ -513,14 +513,6 @@ void main() {
expect(globNode.globNodeState!.inputs, contains(coolAssetId));
expect(globNode.globNodeState!.results, contains(coolAssetId));
await checkChangeType(ChangeType.REMOVE);
expect(

Choose a reason for hiding this comment

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

I don't understand why is this removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be that the asset graph update would invalidate the glob node and modify the result to remove the deleted node; now it just invalidates the glob node, so in this test it's not updated yet.

I have a work-in-progress PR that refactors away from using the graph to track invalidation, at which point this test goes away altogether.

Thanks.

@davidmorgan davidmorgan merged commit d0349c3 into dart-lang:master Apr 30, 2025
74 of 75 checks passed
@davidmorgan davidmorgan deleted the more-inputs-invalidation-test branch April 30, 2025 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants