Skip to content

Vendor eftest and fix #26#46

Merged
camsaul merged 7 commits into
mainfrom
vendor-eftest-and-fix-26
Jun 11, 2026
Merged

Vendor eftest and fix #26#46
camsaul merged 7 commits into
mainfrom
vendor-eftest-and-fix-26

Conversation

@camsaul

@camsaul camsaul commented Jun 11, 2026

Copy link
Copy Markdown
Member

Fixes #26

While fixing this I had to do so much icky monkey patching that I decided the "right" thing to do here would be to just vendor Eftest entirely since we've diverged so much at this point. This let me clean up some existing monkey patching as well.

I renamed eftest.* to mb.eftest.* to prevent issues where both versions are present and tried to delete some stuff we're clearly not using (e.g. Eftest's version of the Junit output or test-var discovery code) but there's a lot more cleanup and consolidation we could do in the future.

More on why I decided to vendor this:

To fix #26 I had to monkey patch eftest.runner/test-vars, a 31-line function that calls a ton of private functions, with basically a line-for-line replacement that looked exactly the same but bound hawk.parallel/*parallel?*... so vendoring made the fix for #26 a one-line change instead of almost 40 once I copied the function over and did the alter-var-root nonsense.

This wasn't the first time we did something like this either, for example in

(def ^:private synchronized? (complement parallel?))
(alter-var-root #'eftest.runner/synchronized? (constantly synchronized?))

we did something similar, which I refactored out in this PR

Overall vendoring lets us eliminate a lot of the monkey patching or duplicate code and should make future changes much easier to do in the future since we can modify the underlying Eftest code without monkey patching and other hacks

Comment thread README.md

## License

Copyright © 2019-2026 James Reeves, 2023-2026 Metabase, Inc.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤝

@camsaul camsaul requested a review from a team June 11, 2026 18:53

@technomancy technomancy left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This looks good but IMO it could use some description of why we decided to vendor this; what our eftest fork does that the upstream doesn't.

@camsaul camsaul merged commit 175e38b into main Jun 11, 2026
2 checks passed
@camsaul camsaul deleted the vendor-eftest-and-fix-26 branch June 11, 2026 21:10
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.

assert-test-is-not-parallel does not work inside fixtures

3 participants