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

Adjust test to pass with slower GHA macOS box #1051

Closed
wants to merge 5 commits into from

Conversation

mceachen
Copy link
Member

This should allow the upgrade to 3.43 to pass:

Failure from GHA:

  1) new Database()
       should accept the "timeout" option:
     AssertionError: expected 1468 to be close to 1000 +/- 200
      at testTimeout (test/10.database.open.js:101:31)
      at Context.<anonymous> (test/10.database.open.js:109:3)
      at process.processImmediate (node:internal/timers:476:21)

https://github.com/WiseLibs/better-sqlite3/actions/runs/5965363532/job/16186656443?pr=1050

@mceachen mceachen requested review from JoshuaWise and a team as code owners August 24, 2023 17:12
@mceachen
Copy link
Member Author

@JoshuaWise this is blocking the upgrade to SQLite 3.43.

expect(() => (this.db = new Database(util.current(), { timeout: -1 }))).to.throw(TypeError);
expect(() => (this.db = new Database(util.current(), { timeout: 75.01 }))).to.throw(TypeError);
expect(() => (this.db = new Database(util.current(), { timeout: 0x80000000 }))).to.throw(RangeError);
for (const { timeout, err } of [
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this offensively worse to read tbh 😄

Copy link
Member Author

@mceachen mceachen Sep 2, 2023

Choose a reason for hiding this comment

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

Yeah, the first time I saw specs written like this, I found it a bit off, as well! Tabular specs have grown on me, though, to the point where I strongly prefer them. It allows you to DRY up your spec, ensures the same operation is being performed with all the different inputs, and most importantly, highlights the input and expected results.

One of the assertions was failing, and I wanted to add the JSON context to it. One solution would be to extract 7 new it's with specific test names, but Josh doesn't follow the one-test-one-spec rule--so I wanted to add the timeout description to the context, but that caused every expectation to linewrap, which was really effective in hiding what the tests were doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

(That said, if @JoshuaWise would rather me rewrite this diff, I don't really care--I just want the PR to go through)

@JoshuaWise
Copy link
Member

JoshuaWise commented Sep 2, 2023

Closing this in favor of 6ee3399. This is also related to #597. I've enabled the long-overdue HAVE_USLEEP=1 compile-time option, which allows the timeout option to be more granular. It turns out that when enabling HAVE_USLEEP=1, the CLI builds for macos-latest take much longer than expected. After investigating, the test times were always 2x-3x the specified timeout. For example, if the timeout was 200ms, it would take 400-600ms; if the timeout was 1000ms, it would take 2000-3000ms. The behavior is quite mysterious, but I can't reproduce it on my local MacOS machine, so I assume it could have something to do with GitHub Actions de-prioritizing virtual machines that are idle. Anyways, it works as intended on all real machines I test it on, so I don't think it's a big deal. In any case, timeouts are never exact in programming—they are always best-effort/minimum times anyways.

@JoshuaWise JoshuaWise closed this Sep 2, 2023
@mceachen mceachen deleted the longer_timeouts branch September 2, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants