Skip to content

Conversation

@jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented Sep 29, 2019

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

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Sep 29, 2019

test('newly added code executes during hot reload', () async {
await _flutter.run();
if (Platform.isWindows) {
Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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....

Copy link
Contributor Author

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.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Sep 30, 2019

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.

@jonahwilliams jonahwilliams changed the title [WIP] Reland: don't update last compile time when compilation is rejected. Reland: don't update last compile time when compilation is rejected. Sep 29, 2019
@codecov
Copy link

codecov bot commented Sep 29, 2019

Codecov Report

Merging #41580 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#flutter_tool 60.61% <100%> (+0.08%) ⬆️
Impacted Files Coverage Δ
packages/flutter_tools/lib/src/devfs.dart 70.19% <100%> (+0.14%) ⬆️
packages/flutter_tools/lib/src/version.dart 45.23% <0%> (-48.1%) ⬇️
packages/flutter_tools/lib/src/build_info.dart 74.59% <0%> (-1.09%) ⬇️
packages/flutter_tools/lib/src/plugins.dart 86.56% <0%> (-1%) ⬇️
packages/flutter_tools/lib/src/cache.dart 45.31% <0%> (-0.67%) ⬇️
packages/flutter_tools/lib/src/vmservice.dart 40.72% <0%> (-0.5%) ⬇️
packages/flutter_tools/lib/src/commands/run.dart 64.39% <0%> (-0.49%) ⬇️
...utter_tools/lib/src/build_runner/build_script.dart
...s/flutter_tools/lib/src/test/flutter_platform.dart 28.05% <0%> (+1.79%) ⬆️
...r_tools/lib/src/runner/flutter_command_runner.dart 73.1% <0%> (+2.52%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d0235...14f5417. Read the comment docs.

);

expect(report.success, false);
expect(devFS.lastCompiled, previousCompile);
Copy link
Member

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.

Copy link
Contributor Author

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

@jonahwilliams jonahwilliams merged commit 39f85f9 into flutter:master Oct 2, 2019
@jonahwilliams jonahwilliams deleted the no_update_failure branch October 2, 2019 19:46
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
…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
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure that compilation errors don't update last compiled time

4 participants