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

grel: Prepare tests for modularization #6571

Merged
merged 2 commits into from May 16, 2024

Conversation

wetneb
Copy link
Sponsor Member

@wetneb wetneb commented Apr 28, 2024

Changes proposed in this pull request:

  • introduce a new base class for GREL tests, which extends our current RefineTest. This makes it possible to remove the test utilities that depend on GREL from the RefineTest class, in anticipation for it being in a core Maven module where GREL isn't available
  • also splits out some tests that were in EvalErrorTests but really belong to the test classes for the functions they actually test. This is also necessary because EvalError will live in the core Maven module (being part of the core data model, as they can be stored in Cell), whereas the corresponding functions will be in the GREL module

@wetneb wetneb requested a review from tfmorris April 28, 2024 09:53
public void tearDown() {
bindings = null;
}

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I have also used this opportunity to clean up unnecessary set up functions which are already included in the base class.

Copy link
Member

@tfmorris tfmorris left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry it took me so long to review. The change summary in the PR was very helpful in wading through the volume of changes to focus on the critical parts, so thanks for that.

public void init() {
functionName = "detectLanguage";
}
String functionName = "detectLanguage";
Copy link
Member

Choose a reason for hiding this comment

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

This seems a little odd, but I guess it comes from the previous code.

@wetneb wetneb merged commit e493b61 into OpenRefine:master May 16, 2024
19 checks passed
wetneb added a commit that referenced this pull request May 16, 2024
Caused by merging #6571 which was already a bit old
@wetneb
Copy link
Sponsor Member Author

wetneb commented May 16, 2024

Merging this broke the build because of a missing import, so I am adding it directly to master with 6a86001

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

2 participants