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

observatory/tests/service/bad_reload_test.dart is broken #34025

Open
aam opened this Issue Jul 30, 2018 · 3 comments

Comments

Projects
None yet
1 participant
@aam
Copy link
Contributor

commented Jul 30, 2018

The test passes, but for a wrong reason.
The test is supposed to confirm VM behavior when hot reload fails. It expects hot reload failure because kernel file produced by frontend for dart source that imports missing source has #main, rather than main entry point. But even if kernel file is correct(if kernel file it has a main entrypoint), the test still passes, indicating that VM still failed to hot-reload. VM failure to hot-reload seems to do with this error:

>testee>err> tag handler failed: 'file:///$DH/sdk/runtime/observatory/tests/service/bad_reload/v2/main.dart' is not a kernel file

Further, the test really need to accommodate improvements in frontend, that would indeed have main, rather than #main entry point in case of import of a missing library source.

cc @peter-ahe-google

@aam aam self-assigned this Jul 30, 2018

@aam aam added the area-vm label Jul 30, 2018

@aam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

The >testee>err> tag handler failed: 'file:///$DH/sdk/runtime/observatory/tests/service/bad_reload/v2/main.dart' is not a kernel file is happening since dart switched to dart2 mode by default. When you force dart1 mode(fullArgs.add('--no-preview-dart-2'); at /runtime/observatory/tests/service/test_helper.dart:147), that error disappears.

@aam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

There is a real problem in hot reload in vm where when loading from kernel, we don't correctly recognize the case when root library is changing.

@aam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

Beyond fixing hot reload when uri changes(which should be fixed by https://dart-review.googlesource.com/c/sdk/+/67400) there is a problem with how compiler is not reset if attempt to hot-reload fails on VM. When VM fails to hot-reload and rejects the patch, compiler is not really reset to last-known-green-state, so attempt to compile expression will work on updated compiler, instead of revert one.

dart-bot pushed a commit that referenced this issue Aug 16, 2018

[vm] Fix hot reload when root script changes.
This addresses #34025 (comment).

Change-Id: Ic3bfaaf9a43f42ffff15c5d8a8317dab83569b98
Reviewed-on: https://dart-review.googlesource.com/67400
Commit-Queue: Alexander Aprelev <aam@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>

dart-bot pushed a commit that referenced this issue Sep 5, 2018

When compiled delta is rejected, reset incremental compiler back to l…
…ast known good state(last accepted state).

This is needed so that compileExpression requests are serviced from that accepted state.

Bug: #34025
Change-Id: I592d2af50e59a721e1feb1699c6d56bcd568465f
Reviewed-on: https://dart-review.googlesource.com/72540
Commit-Queue: Alexander Aprelev <aam@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.