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

Add an 'updateResult' utility function to upsert values into data-* files #1809

Open
wants to merge 7 commits into
base: gh-pages
Choose a base branch
from

Conversation

robhogan
Copy link

@robhogan robhogan commented May 9, 2022

As far as I could tell, the current accepted practice to update data- files is to use a script to suggest the changes to a console and then to make manual updates.

This is a lot of work for new runtimes, and doesn't lend well to automating as part of a runtime release process.

This is a simple utility to parse and update res objects in place. It may be called from the test runner scripts - I used it to generate #1808.

Because it uses an actual parser it should be robust to syntax gotchas like code-in-strings, but it makes a few basic assumptions that hold for the data files as they are now, like:

  • Data files have an exports.tests = [ /* ... */ ] section
  • Tests must have an object-literal res property, or an array-literal subtests property.

@ljharb
Copy link
Member

ljharb commented May 9, 2022

Most runtimes we support are browsers, so the primary workflow is to manually update results as they’re checked. Command-line runtimes sometimes have a script that updates the files automatically.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this intended to be used, exactly?

package.json Outdated Show resolved Hide resolved
@robhogan
Copy link
Author

robhogan commented May 9, 2022

Thanks for looking at this so quickly. I work with the Hermes engine team so we have an interest in (semi-)automating updates to compat-table as part of the engine release process.

I've pushed my changes to hermes.js to show how I've used the utility - basically we modify the data file in place on a compat-table checkout. The changes can be reviewed by eye, footnotes manually added if necessary, and then pushed up for a PR. It eliminates the error-prone step of taking the console output and manually transcribing the results into the data files.

@robhogan
Copy link
Author

robhogan commented May 9, 2022

I’d say so. Happy to update those too if the approach looks good to you.

@robhogan
Copy link
Author

robhogan commented May 9, 2022

(Done)

hermes.js Show resolved Hide resolved
hermes.js Outdated Show resolved Hide resolved
jerryscript.js Outdated Show resolved Hide resolved
nashorn.js Outdated Show resolved Hide resolved
rhino.js Outdated Show resolved Hide resolved
update-result.js Outdated Show resolved Hide resolved
update-result.js Outdated Show resolved Hide resolved
update-result.js Show resolved Hide resolved
update-result.js Show resolved Hide resolved
@robhogan
Copy link
Author

robhogan commented May 9, 2022

Thanks for the review @ljharb - the reason for the retro syntax though is the jshint lint setup for this project. For example, applying one of the suggestions above (which I'd normally very much agree with!):

> npm run lint
update-result.js: line 147, col 52, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').

I'm not exactly sure what that implies about runtime feature support, but I've tried to be as conservative as I remember how to be (my ES3 is a little rusty!), mostly for consistency's sake - hence avoiding Array.from, repeat etc - it feels a bit awkward to use newer runtime features while working with a linter that forbids const.

These constraints make sense in the test code where we obviously need to cater for the lowest common denominator, but probably shouldn't apply to scripts written for Node JS, like this one or the various engine harnesses. I'm not sure how best to reconcile that - different lint settings for test code vs scripts? Different linter? Exclude scripts from lint?

@ljharb
Copy link
Member

ljharb commented May 9, 2022

ha, ok fair enough :-)

just ignore those suggestions for now; we should update from jshint to eslint anyways, and then we can use overrides so that scripts like this don't have to meet the same standards.

@robhogan
Copy link
Author

robhogan commented May 9, 2022

Sounds good to me - I'll assume modern runtime support and apply all the suggestions the linter is happy with

@robhogan robhogan requested a review from ljharb July 19, 2022 19:40
@p-bakker
Copy link

p-bakker commented Oct 9, 2023

Not sure why this PR went nowhere, but #1881 achieves the same, without having to use a parser to modify the module code AND will allow custom Compat Tables, like the (vary outdated) custom compat Table we have at https://rhino.github.io/compat/engines.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants