Vendor eftest and fix #26#46
Merged
Merged
Conversation
|
|
||
| ## License | ||
|
|
||
| Copyright © 2019-2026 James Reeves, 2023-2026 Metabase, Inc. |
technomancy
approved these changes
Jun 11, 2026
technomancy
left a comment
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.*tomb.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 boundhawk.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 thealter-var-rootnonsense.This wasn't the first time we did something like this either, for example in
hawk/src/mb/hawk/parallel.clj
Lines 15 to 17 in a8f3e8a
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