-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Reland: don't update last compile time when compilation is rejected. #41580
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
Reland: don't update last compile time when compilation is rejected. #41580
Conversation
|
|
||
| test('newly added code executes during hot reload', () async { | ||
| await _flutter.run(); | ||
| if (Platform.isWindows) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current guess is that slight changes in the recorded timestamp have tripped up the windows filesystem. Since the files are modified synchronously in the test environment, perhaps the last recorded timestamp is identical to the first compiled time?
I tried a few different approaches like keeping the datetime initialization before the call to compile and only rolling it back afterwards, but that wasn't sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh fails on CI, passes locally ... more time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the creation of the lastCompiledTime all the way back to the start of the method... that plus the awaits should help....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the awaits, that was all that we needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue was likely that moving DateTime.now() to after the compilation placed it closed enough to the next invalidation that the with the windows file-system the timestamp granularity was not sufficient.
Codecov Report
@@ Coverage Diff @@
## master #41580 +/- ##
==========================================
+ Coverage 60.52% 60.61% +0.08%
==========================================
Files 193 192 -1
Lines 18891 18784 -107
==========================================
- Hits 11434 11385 -49
+ Misses 7457 7399 -58
Continue to review full report at Codecov.
|
| ); | ||
|
|
||
| expect(report.success, false); | ||
| expect(devFS.lastCompiled, previousCompile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this test checks that the correct value of lastCompiled is retained, but it would probably also be good to have a test that demonstrates why retaining the correct value is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated with a test case that confirms that the lastCompiled time is updated on success. Correctly using that value is a bit out of scope, since that is only used by the resident_runner
…lutter#41580) * dont update last compiled time when compilation is rejected * lets try flushing, thats a neat trick * windows man * Update hot_reload_test.dart * Update hot_reload_test.dart * Update devfs.dart * Update hot_reload_test.dart * Update hot_reload_test.dart * add test that verifies when compile is good that time is updated * Update devfs_test.dart
Description
If we reject the compilation, keep the source files invalidated until they are updated. This keeps the error if the user repeatedly recompiles with an exception
Fixes #41399