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 docstrings tests with mocha #7220

Merged
merged 13 commits into from
Dec 30, 2024

Conversation

aspeddro
Copy link
Contributor

@aspeddro aspeddro commented Dec 28, 2024

Main changes:

  • Generates a single file with all the examples from the docstrings, generated_mocha_test.res
  • Runtime tests using mocha
  • Much faster
  • Run on macos and Linux

@aspeddro aspeddro changed the base branch from doc-tests-fix to master December 28, 2024 01:01
@aspeddro aspeddro changed the base branch from master to doc-tests-fix December 28, 2024 02:10
@aspeddro aspeddro marked this pull request as ready for review December 28, 2024 02:11
scripts/test.js Outdated Show resolved Hide resolved
scripts/test.js Outdated Show resolved Hide resolved
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Great work! This is awesome. Huge speedup (> 10x on my machine).

Some feedback:

  1. generated_mocha.test.res is quite huge. Maybe we shouldn't check it in?
  2. If possible, could we change the generation so that we get one describe call per module (e.g., String) with multiple test calls inside (one per function, e.g., charAt, charCodeAt, ...).
  3. Is the inner module Test actually needed? In what cases?

scripts/test.js Outdated Show resolved Hide resolved
scripts/test.js Outdated Show resolved Hide resolved
| None => []
}
// Ignore some tests not supported by node v18
let ignoreRuntimeTests = [
Copy link
Member

Choose a reason for hiding this comment

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

Bonus points: detect node version and run tests accordingly. As these tests actually work with Node 22.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

let main = async () => {
let examples = await extractExamples()
examples->Array.sort((a, b) =>
Obj.magic(a.id) > Obj.magic(b.id) ? Ordering.fromInt(1) : Ordering.fromInt(-1)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need Obj.magic here?

I think this can just be

  examples->Array.sort((a, b) => String.compare(a.id, b.id))

and should be moved into the extractExamples function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@aspeddro aspeddro changed the base branch from doc-tests-fix to master December 29, 2024 19:29
@aspeddro aspeddro changed the base branch from master to doc-tests-fix December 29, 2024 19:30
@aspeddro
Copy link
Contributor Author

Great work! This is awesome. Huge speedup (> 10x on my machine).

Some feedback:

  1. generated_mocha.test.res is quite huge. Maybe we shouldn't check it in?
  2. If possible, could we change the generation so that we get one describe call per module (e.g., String) with multiple test calls inside (one per function, e.g., charAt, charCodeAt, ...).
  3. Is the inner module Test actually needed? In what cases?
  1. Added generated_mocha_test.res and generated_mocha_test.res.mjs to gitignore.
  2. Done
  3. Yes, bc some examples contains type definitions. Added comment on code

@aspeddro aspeddro requested a review from cknitt December 29, 2024 21:06
@cknitt
Copy link
Member

cknitt commented Dec 30, 2024

  1. Done

Doesn't really seem to be done yet? I still see one describe per test.

But never mind, we can always improve things in separate PRs.

@cknitt cknitt merged commit 15e4b91 into rescript-lang:doc-tests-fix Dec 30, 2024
@cknitt
Copy link
Member

cknitt commented Dec 30, 2024

Doesn't really seem to be done yet? I still see one describe per test.

Sorry, please disregard, seems I somehow had a stale generated file locally. 🤦

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.

2 participants