-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Run wasm console sample on Helix #45768
Run wasm console sample on Helix #45768
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
14aef9d
to
12590c8
Compare
87bfb37
to
6e81506
Compare
d92d6d6
to
a897d2c
Compare
a5709f3
to
10540ee
Compare
@MaximLipnin does the sample run on helix now? Can you share a link to the log? I wasn't able to get to it. |
@radical yeah, it should, please take a look at logs: |
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.
@akoeplinger would be able to review this well. I don't know enough about the helix related bits.
- I'm wondering if we can/should be generating
RunTests.sh
. - We could make this handle more samples, or similar projects too, but that doesn't have to be in this PR.
- In general, this looks fine, and it seems to work!
- It should be useful to get this in anyway, as it will prevent samples getting unintentionally broken.
That is only done by the libraries tests afaik. |
This is done here. You could technically import these targets for the sample project and just hook them up: runtime/eng/testing/tests.targets Lines 51 to 52 in 93e767b
I think I would rather do that and try to be consistent, it seems cumbersome to have that "workaround" in sendhelix to include the sample as part of the payload and hardcode the Can we open a follow up issue to clean this up and make it more generic for more samples? I see we also added this for iOS and it is doing somewhat similar instead of use the targets we already have. |
Relates to #43865