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

Test optimization levels #1317

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Test optimization levels #1317

merged 4 commits into from
Apr 25, 2023

Conversation

rbri
Copy link
Collaborator

@rbri rbri commented Apr 10, 2023

No description provided.

@p-bakker
Copy link
Collaborator

p-bakker commented Apr 10, 2023

Could you elaborate a bit on the purpose of this PR?

@rbri
Copy link
Collaborator Author

rbri commented Apr 10, 2023

The idea is to make sure we get the same result when executing js stuff with different optimization levels.
(My next pr is exactly one case where we get different results at the moment)

@rbri
Copy link
Collaborator Author

rbri commented Apr 11, 2023

@p-bakker ok for you?

@p-bakker
Copy link
Collaborator

Mmm, not sure this is the right approach, adding the logic to run the tests at different optimization levels to the test itself.

We have all the logic to run tests at the different optimization levels in the TestRunners I think, see https://github.com/mozilla/rhino/blob/master/testsrc/README.md#optimization-levels. And at least when running Test262 with the option enabled to update the test262.properties file, the tests are run at optimization levels -1, 0 and 1 and for a test to pass it must pass at all three tested optimization levels.

So the approach to code inside the test itself to run against multiple optLevels seems not correct to me and will cause the tests to run at the tested optLevels multiple times when the MozillaSuiteTest is used, as that already runs the tests at multiple optLevels.

Instead, for our own tests (MozillaSuiteTest) maybe we want to have an option to somehow combine the results of the three runs at different optLevels and pass or fail it once.

@rbri
Copy link
Collaborator Author

rbri commented Apr 11, 2023

Ah now i see you point, but this is more a general problem with the test suite (i noted parts of the problem also while working on this).
But this is only about this special test cases - do you think they are running already with different optimization levels?

Maybe i will try to cleanup the test suit regarding this (step by step).

@p-bakker
Copy link
Collaborator

Haven't double-checked it now, but from what I recall running ./gradlew test does run both Test Suites at optLevel -1, 0 and 1. This is also what it says at https://github.com/mozilla/rhino/blob/master/testsrc/README.md#running-tests

@p-bakker
Copy link
Collaborator

p-bakker commented Apr 11, 2023

And I know for sure that the update test262.properties option only marks a particular test as passed in the .properties file if it passes all three tested optLevels (and if it doesn't pass on a particular optLevel but passes on other optLevels, it'll update the comment on the .properties file accordingly)

@rbri
Copy link
Collaborator Author

rbri commented Apr 11, 2023

ok, will have a look

@rbri
Copy link
Collaborator Author

rbri commented Apr 11, 2023

And I know for sure that the update test262.properties option only marks a particular test as passed in the .properties file if it passes all three tested optLevels

Yes that's also my understanding, same for all test annotated @rhinotest (but we have to at least sync the test levels used)

use Utils.runWithAllOptimizationLevels at more places
@gbrail
Copy link
Collaborator

gbrail commented Apr 25, 2023

Sorry it's been such a long time -- some of the big frameworks like test262 do indeed run bunches of tests at multiple optimization levels but many more tests do things differently and it's inconsistent. If this PR makes more tests work at different optimization levels then it seems like a pretty good idea to fix!

@gbrail gbrail merged commit 6387bac into mozilla:master Apr 25, 2023
3 checks passed
@rbri rbri deleted the testOptimizationLevels branch May 30, 2023 16:14
johnspackman added a commit to johnspackman/rhino that referenced this pull request Dec 25, 2023
…0917

* commit '6387bac4281ba1c4a8050cd055071615593d214c': (78 commits)
  More consistent test optimization levels (mozilla#1317)
  ci: set minimal permissions on GitHub Workflows
  Compare all primitive value type wrappers by value
  Fixing formatting issues
  Treat String, ConsString, Boolean, and Double as value types
  setter function (from property descriptor) has to convert the args (see HtmlUnit#7) also
  use try-with-resource
  FIX: Wrap key and value for NativeJavaMap iterator
  Added more tests
  FIX: Wrap result of iterator
  BUG: for X of javaList does not work properly in strict mode
  removed obsolete LineSeparator
  Spotless
  Preserving cause, when a JavaException is rethrown in JavaScript
  fix ScriptException when bound functions are called inside Promise.then()
  test cases from mozilla#1242 fix taken from p-bakker's sugestion
  fix name property for bound functions (see issue mozilla#1297)
  Fix the condition for isResourceChanged
  Code Cleanup (mozilla#1295)
  ignore %c styling but count to not disturb further replacements
  ...

# Conflicts:
#	src/org/mozilla/javascript/NativeObject.java
#	testsrc/org/mozilla/javascript/tests/json/JsonParserTest.java
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