-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add Dart to executors #65
Conversation
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.
A few comments and notes. I don't know enough about Dart to review in more detail.
@@ -0,0 +1,191 @@ | |||
/* Main execution program for Data Driven Test of ICU / Unicode / CLDR |
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 is just a copy of the node executor, I don't know what the smartest way is to not duplicate the file. Copying at runtime seems ugly...
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.
OK for now, but copy will be a problem when the node version gets out of sync with Dart implementation.
@mosuem The PR looks fine in theory, so if we can get this working, we can merge. Note: I don't know yet if this will update our dashboard HTML templates to include the Dart {Web, Native} test results in the dashboards, but I think that is a good for a followup PR, not here. I checked out the PR (
Can you help resolve this? I don't know if we can count on Dart being installed on the Github Actions CI's runner VMs by default. Or, at least, I don't currently know how to find out which things get pre-installed. The safest way is to use the Github Action by the Dart Lang team that does the setup of Dart within a CI workflow. I think what you need to do is edit
|
@echeran I updated the Dart version, and added it to the Github CI pinned by a hash (see https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions). |
…into dartExecutors
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.
With the exception of copying the executor.js file, this looks OK to me.
Any idea on how to better fix this, especially until all |
Hey, please resolve the conflict with testdriver/datasets.py |
@sven-oly Could you approve the running of workflows? |
Shane or Elango, please help with the workflow. I don't see a way for me to approve as Mortiz requested |
Just testing collators on the web for now; as ICU4X integration progresses native tests will be possible.