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

Adding tests for cross-module constant equality after hot restart in DDC #2349

Merged
merged 14 commits into from
Jan 24, 2024

Conversation

Markzipan
Copy link
Contributor

@Markzipan Markzipan commented Jan 19, 2024

These tests pass today but fail if certain constant-holding containers are cleared after a hot restart in DDC (see this bug.

Additional changes:

  • Adds makeEditToDartLibFile, a helper function which lets us edit non-main-module files.
  • Adds two hot restart test fixtures
  • Sets both hot restart tests to run daily

Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Very cool, thanks @Markzipan for porting this over!

fixtures/_testHotRestart1/lib/library1.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/project.dart Outdated Show resolved Hide resolved
dwds/test/fixtures/project.dart Show resolved Hide resolved
Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@nshahan
Copy link
Contributor

nshahan commented Jan 19, 2024

I don't see the new test as failing on the presubmit here so I'm not sure if this is fully triggering all the behavior we need to to verify a fix in the Dart SDK.

Oh I see. This is intended to be a regression test to catch if we break expected behavior when clearing constant caches. It should be passing now and with any future fixes, that makes sense now.

So we should add a new test that fails now because of the behavior that we observed in a flutter app where the stale const from before the hot restart remained live in the UI.

@Markzipan
Copy link
Contributor Author

Markzipan commented Jan 22, 2024

I added additional cases, I'm having trouble reproducing the flutter case. IIRC we have 2 cases we need to ultimately test:

  • Not clearing consts at all, which causes the "flutter issue", whereby hot reload reuses old consts.
  • Clearing consts without clearing constant containers, which will cause certain constant comparisons to fail (tested in this PR).

Edit: following up here - the issue was that my SDK was checked out at a version prior to turning on the new type system by default.

Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Thanks @Markzipan - I added a note below which I think could be the reason why you have not been able to repro the "flutter issue". Let me know if that addresses the mismatch.

fixtures/_testHotRestart2Sound/web/main.dart Outdated Show resolved Hide resolved
fixtures/_testHotRestart2Sound/web/main.dart Outdated Show resolved Hide resolved
Copy link
Member

@sigmundch sigmundch left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the requests, LGTM (just a minor comment nit below)

fixtures/_testHotRestart2Sound/web/main.dart Outdated Show resolved Hide resolved
@Markzipan Markzipan merged commit 37f5276 into dart-lang:master Jan 24, 2024
47 checks passed
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.

4 participants