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

Run Test262 tests against production artifacts. #41

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

12wrigja
Copy link
Contributor

@12wrigja 12wrigja commented Sep 3, 2021

Fixes #4, though there are test failures in both the original repo and in this one which should probably get fixed up before using this as part of CI.

@12wrigja 12wrigja force-pushed the test262 branch 3 times, most recently from 56e077f to 51604b3 Compare September 3, 2021 23:59
@12wrigja
Copy link
Contributor Author

12wrigja commented Sep 4, 2021

This relies on tc39/proposal-temporal#1801, which is the cleanest way I can think of to allow other Temporal implementations to re-use the Test262 test setup from the proposal.

Long-term, is it expected that the 262 test still live in that repo or will they be copied out to some standard repo?

@ptomato
Copy link
Contributor

ptomato commented Sep 4, 2021

They'll be copied into https://github.com/tc39/test262 as soon as all of the Mocha-style tests have been converted to use the test262 style.

@12wrigja
Copy link
Contributor Author

12wrigja commented Sep 9, 2021

I've merged in #42 into this draft PR, but will rebase these changes later once it's merged into main, as the some of tests in the proposal repo need those changes to pass.

There's 6 failing tests that I'm trying to look into, but I could definitely use some pointers on how to efficiently debug them.

Is there a way to force the tests to run on CI even for draft PR's? This should generate logs that might be useful to help guide my fixing. :)

@justingrant
Copy link
Contributor

I've merged in #42 into this draft PR, but will rebase these changes later once it's merged into main, as the some of tests in the proposal repo need those changes to pass.

@Ms2ger just merged that PR this morning (Thanks!), which may make it easier to finish this PR.

Is there a way to force the tests to run on CI even for draft PR's? This should generate logs that might be useful to help guide my fixing. :)

I think it's fine to convert from draft to normal PR if that makes it easier. But I did find a bunch of people documenting how to prevent tests from being run on draft PRs, which implies to me that there must be (or at least must have been!) a way to run GH Actions on a draft PR.

@justingrant
Copy link
Contributor

There's 6 failing tests that I'm trying to look into, but I could definitely use some pointers on how to efficiently debug them.

Are those failures related to tc39/proposal-temporal#1811 ?

@12wrigja
Copy link
Contributor Author

12wrigja commented Sep 9, 2021

I don't think so - though those also show up. These 6 failures are different. I managed to get the CI working, but the logs seem to be truncated, so here's what I see when I run the tests locally:

6 tests failed:

FAIL Calendar/prototype/dateAdd/balance-smaller-units.js (strict mode)

evalmachine.<anonymous>:1411
  throw new Test262Error(message);
  ^

Test262Error {
  message: 'units smaller than days are balanced day result Expected SameValue(«3», «9») to be true'
}

FAIL Calendar/prototype/dateAdd/duration-argument-string-negative-fractional-units.js (strict mode)

evalmachine.<anonymous>:1411
  throw new Test262Error(message);
  ^

Test262Error {
  message: 'negative fractional hours day result Expected SameValue(«2», «1») to be true'
}



FAIL PlainDate/prototype/add/balance-smaller-units.js (strict mode)

evalmachine.<anonymous>:1411
  throw new Test262Error(message);
  ^

Test262Error {
  message: 'the duration is passed directly to the calendar Expected SameValue(«[object Object]», «P1DT24H1440M345600S») to be true'
}



FAIL PlainDate/prototype/subtract/balance-smaller-units.js (strict mode)

evalmachine.<anonymous>:1380
  throw new Test262Error(message);
  ^

Test262Error {
  message: 'the duration is negated but not balanced before passing to the calendar instanceof'
}



FAIL PlainYearMonth/prototype/add/calendar-arguments.js (strict mode)

evalmachine.<anonymous>:1411
  throw new Test262Error(message);
  ^

Test262Error {
  message: 'args[1] Expected SameValue(«[object Object]», «[object Object]») to be true'
}



FAIL PlainYearMonth/prototype/subtract/calendar-arguments.js (strict mode)

evalmachine.<anonymous>:1411
  throw new Test262Error(message);
  ^

Test262Error {
  message: 'args[1] Expected SameValue(«[object Object]», «[object Object]») to be true'
}

justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 9, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
@justingrant
Copy link
Contributor

I could definitely use some pointers on how to efficiently debug them.

I got inspired to make debugging easier, so if you use the VS Code debugger, #46 will provide one-click debugging support in the IDE for both Demitasse and Test262 tests. The latter will require tc39/proposal-temporal#1812

Once the tests can be stepped through in the debugger, it made them easier to understand what's going on! I only looked at one of the failures (PlainYearMonth/prototype/subtract/calendar-arguments.js) and I didn't know why we were comparing two different empty objects and expecting them to be identical. I assume there's some commit(s) from proposal-temporal that we're missing here?

@justingrant
Copy link
Contributor

BTW, one thing it'd be great to fix before merging this: when the proposal-temporal code is brought over to run tests, it causes error modals to show up in VSCode because there's some conflict in ESLint and Prettier. Here's the error I get any time I open or edit a file inside the tc39 folder: (maybe outside it too?)

Error: Plugin "prettier" was conflicted between "test/tc39/.eslintrc.yml" and "../.eslintrc.yml".

I assume there must be some setting to prevent this. Maybe revising to eslint/prettier ignore files or configs in this repo? If not, then I assume we can always apply changes after merging in the proposal-temporal code, e.g. removing or changing proposal-temporal's eslint and/or prettier configs as a patch after merging? Regardless, I really don't want to deal with those annoying dialogs every day!

BTW, changing the annoying modal dialog UI (for all ESLint crashes, not just this one) is tracked at microsoft/vscode-eslint#1238. So it's possible this might get "fixed" without us having to change anything. But the last I heard, the plugin maintainer is not on board with changing this behavior. So I think we should assume that we'll have to fix on our end.

@justingrant
Copy link
Contributor

BTW, here's the full call stack of the ESLint error above. Maybe it's the ignore files themselves which might be conflicting?

at mergePlugins (/Users/justingrant/Documents/hdev/temporal-polyfill-ts/temporal-polyfill/temporal-polyfill/node_modules/@eslint/eslintrc/lib/config-array/config-array.js:202:19)
    at createConfig (/Users/justingrant/Documents/hdev/temporal-polyfill-ts/temporal-polyfill/temporal-polyfill/node_modules/@eslint/eslintrc/lib/config-array/config-array.js:305:9)
    at ConfigArray.extractConfig (/Users/justingrant/Documents/hdev/temporal-polyfill-ts/temporal-polyfill/temporal-polyfill/node_modules/@eslint/eslintrc/lib/config-array/config-array.js:481:33)
    at CLIEngine.isPathIgnored (/Users/justingrant/Documents/hdev/temporal-polyfill-ts/temporal-polyfill/temporal-polyfill/node_modules/eslint/lib/cli-engine/cli-engine.js:974:18)
    at CLIEngine.executeOnText (/Users/justingrant/Documents/hdev/temporal-polyfill-ts/temporal-polyfill/temporal-polyfill/node_modules/eslint/lib/cli-engine/cli-engine.js:884:38)
    at _e.lintText (/Users/justingrant/.vscode/extensions/dbaeumer.vscode-eslint-2.1.25/server/out/eslintServer.js:1:177327)
    at /Users/justingrant/.vscode/extensions/dbaeumer.vscode-eslint-2.1.25/server/out/eslintServer.js:1:171074
    at Re (/Users/justingrant/.vscode/extensions/dbaeumer.vscode-eslint-2.1.25/server/out/eslintServer.js:1:177178)
    at /Users/justingrant/.vscode/extensions/dbaeumer.vscode-eslint-2.1.25/server/out/eslintServer.js:1:171032
    at /Users/justingrant/.vscode/extensions/dbaeumer.vscode-eslint-2.1.25/server/out/eslintServer.js:1:172452

test/test262.sh Outdated Show resolved Hide resolved
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
justingrant added a commit to justingrant/temporal-polyfill that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in js-temporal#41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
12wrigja pushed a commit that referenced this pull request Sep 10, 2021
This commit adds a launch.json to enable easy IDE debugging of either
Demitasse or (coming soon in #41) Test262 tests.

For Test262, there are two different configs:
1) Only debug the subset of tests shown in launch.json. Edit launch.json
   to debug other tests. Warning: commenting out the TESTS env var
   will debug all tests but that will take 10+ mins to run.
2) Only debug the test file currently open in the editor.
lib/init.ts Show resolved Hide resolved
test/test262.sh Show resolved Hide resolved
@12wrigja
Copy link
Contributor Author

I assume there's some commit(s) from proposal-temporal that we're missing here?

It's actually the opposite: bafa1bd broke this, wasn't ported to proposal-temporal as far as I can tell, where presumably the test would have failed. Should that commit be rolled back, or should the test change? I could certainly see a use for a test that verifies that some equivalent object is passed, but as far as I can tell the Test262 assertion library is... lacking in that regard.

@12wrigja
Copy link
Contributor Author

Right - finally had more time to look at this.

After #48 is merged, there's only one test failure, and I explained it above.

Should the test itself change (and the commit backported), or should bafa1bd be dropped?

@cjtenny as you authored that commit - what do you suggest?

@12wrigja
Copy link
Contributor Author

@justingrant R.e. your issues with plugins: what plugin(s) are you using? I've also been using VSCode to work on this, and I haven't seen any such popup (though I'm guessing it's because I don't have the right plugins installed - I generally use VSCode as a glorified text editor w/ TS Language features and leave the linting and such to CLI tooling, as it's not very well integrated at Google).

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 16, 2021

I assume there's some commit(s) from proposal-temporal that we're missing here?

It's actually the opposite: bafa1bd broke this, wasn't ported to proposal-temporal as far as I can tell, where presumably the test would have failed. Should that commit be rolled back, or should the test change? I could certainly see a use for a test that verifies that some equivalent object is passed, but as far as I can tell the Test262 assertion library is... lacking in that regard.

Apparently this is a port of tc39/proposal-temporal#1748 which hasn't landed yet.

@12wrigja 12wrigja marked this pull request as ready for review September 18, 2021 02:49
@12wrigja
Copy link
Contributor Author

@justingrant I tried installing both the eslint and prettier plugins, but as far as I can tell they both seem fine when opening up various files. Maybe since you tried this I had added the tc39 folder to .gitignore and that helped?

I'm going to merge, this but if you continue to see this issue please file another issue and I'll take a look next week.

@12wrigja 12wrigja merged commit f885253 into js-temporal:main Sep 18, 2021
@justingrant
Copy link
Contributor

@justingrant I tried installing both the eslint and prettier plugins, but as far as I can tell they both seem fine when opening up various files. Maybe since you tried this I had added the tc39 folder to .gitignore and that helped?

I'm going to merge, this but if you continue to see this issue please file another issue and I'll take a look next week.

Sounds good. Thanks for getting this working!

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.

How to run Test262 on this polyfill?
4 participants