-
Notifications
You must be signed in to change notification settings - Fork 97
[record_use] Integration test case for library-uris #2987
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
Conversation
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with |
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.
This hook seems to really be a test. Does it run on CI? It wasn't immediately clear to my what triggered it there...
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.
Might also be worthwhile to document on top of this file that really, it's the test.
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.
We will trigger it on the Dart CI. This is a test data project. I'm working on
the Dart SDK CL.
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.
Let's document for future readers here (and maybe on the pubspec) that that's how it is run on CI.
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.
All test_data/ projects in pkgs/hooks_runner/ follow the same project btw.
| For dart2js, `dart compile js --write-resources` and inspect the contents of | ||
| the file. | ||
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.
Soo... is just just a manual test? Can we automate it (i.e. the inspecting of the content of the file) and run it on CI?
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.
We will automate it inside the Dart SDK, not on dart-lang/native. (Running it
here would be bad, we would see fixes and breakages only on dev and stable
releases.)
|
General thought: |
| positionalArguments: [null], | ||
| namedArguments: {'bar': null}, |
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.
Let's include an additional test with multiple arguments for both? (The current one wouldn't have caught the , typo)
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.
Let's document for future readers here (and maybe on the pubspec) that that's how it is run on CI.
It's for debugging, that is what I was doing. I don't want to add something for users, but we can't not do it if we want debugging info ourselves. |
|
I can also remove all the toString stuff and let Gemini generate it again the next time I need it 😆 It's not the point of this PR. |
goderbauer
left a comment
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.
LGTM
A test case to use for
The test case checks:
lib/(should be package uri)lib/(should be package uri)lib/(what should the canonical uri be of this? I left an open TODO.)lib/(should be package uri)lib/(should be package uri)lib/(what should the canonical uri be of this? I left an open TODO.)See the linked issue for the current state of the VM and dart2js implementation.