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

Fix unit tests to be fully portable #14

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Conversation

UnasZole
Copy link

@UnasZole UnasZole commented Jan 14, 2024

After checking first checking out the project, I originally got 457 unit test failures due to environmental issues.
This PR attempts at fixing most of them.

  • Enforce en-US locale for test execution : fixes ~331 tests
  • Remove android logger dependency (as a library, jsword should only rely on the slf4j api, and let the final application provide the logger implementation) : fixes ~78 tests
  • Automatically skip tests that rely on externally-installed data if this data is not available, and remove download of ESV2011 which is missing from crosswire repo : fixes ~46 tests

The numbers are approximate, because in the middle of that there are also some flaky tests remaining (that pass or fail a bit randomly) : specifically the JobTest seems to consistently pass within my IDE, but randomly fail when run from Gradle.

Notes :

  • The only change to the actual jar is the logger dependency update : the lib now only depends on slf4j API, and leaves it up to the client application to provide a correct slf4j implementation. That means the "slf4j-android-logger" dependency might need to be added to and-bible if it's not already there.
  • Ideally, tests that rely on external data should be rewritten to rely only on data included within the src/test/resources folder. In particular for tests relying on modules no longer distributed by CrossWire (eg ESV2011).

…ly after checking out the project.

- Enforce en-US locale for test execution : fixes 331 tests
- Remove android logger dependency (as a library, jsword should only rely on the slf4j api) : fixes 78 tests
- Automatically skip tests that rely on externally-installed data if this data is not available, and remove download of ESV2011 which is missing from crosswire repo : fixes 46 tests
@UnasZole
Copy link
Author

The second commit of this PR brings a change to BackendTest.testBackendStrongsGreekRawLd() which might require additional action to pass on pipeline builds.

Indeed, it relies on StrongsGreek, but it seems that the StrongsGreek package currently available in CrossWire has been updated compared to the original test, as the content no longer matched.
Therefore, I updated the test method to accept the contents of the current StrongsGreek package - but as a result, the test might fail with the older version of StrongsGreek that is shipped in your testmods.zip, which I don't have access to.

If we notice that the test fails on the pipeline, two solutions :

  • Either simplify the test (ie just check for the presence of the number "3588" and the word "ho", which are in both versions)
  • Or update your testmods.zip to use the latest StrongsGreek package version.

if(project.hasProperty("tests")) {
implementation("org.slf4j:slf4j-api:1.7.6")
} else {
implementation("de.psdev.slf4j-android-logger:slf4j-android-logger:1.0.5")
Copy link

Choose a reason for hiding this comment

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

When using in Android app (AndBible) we need this logger implementation.

Copy link

Choose a reason for hiding this comment

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

Could you provide PR for AndBible how to use this if we make this change?

Copy link
Author

@UnasZole UnasZole Jan 16, 2024

Choose a reason for hiding this comment

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

Well, I'm not familiar with android development (I'm a very experienced Java developer, but only for backend applications, not frontends so far) so I'm not equipped to run tests on and-bible. (I could take time to try it later on, though).

But the change is very simple : you can just add the dependency directly in this list : https://github.com/AndBible/and-bible/blob/develop/app/build.gradle.kts#L335
This should be enough :-)

…eferenced by tests and still available on CrossWire, in order to un-skip the corresponding tests
@UnasZole
Copy link
Author

UnasZole commented Jan 17, 2024

I've just updated the PR to simplify the testBackendStrongsGreekRawLd, to avoid the possible issue I mentioned above (due to the fact that the StrongsGreek module this test relies on has changed between what's currently available on CrossWire, and what's in your pipeline's testmods.zip).

Now, the test should pass in both cases, so you won't need to update testmods at all :-)

Could you approve the Workflow that is pending, so we can check if all the unit tests still pass in your test environment ?
(And then merge, if it's successful ! ;-) )

I'll open the PR to and-bible to update the dependencies to this new jsword version (and to the slf4j android logger) once it's published, if you want :-)

@tuomas2
Copy link

tuomas2 commented Jan 18, 2024

It looks good to me. Will merge and see if I get it working in AndBible side.

@tuomas2 tuomas2 merged commit 8743b57 into AndBible:master Jan 18, 2024
1 check failed
@tuomas2
Copy link

tuomas2 commented Jan 18, 2024

Interesting. It looks like it works without any changes in AndBible (and checked that Slf4jAndroidLogger log lines get into logcat). I wonder if de.psdev.slf4j-android-logger is obsoleted by standard android libs or more recent version of slf4j.

@UnasZole
Copy link
Author

The android logger slf4j implementation is probably already included transitively in some other dependencies of and-bible !

Thank a lot for the merge !

By the way, I saw that the CI build on the pull request had failed (cf. https://github.com/AndBible/jsword/actions/runs/7560282128/job/20605220349 ), apparently because it didn't manage to retrieve the URL of the testmods file (the secrets.DOWNLOAD_TEST_MODULES_URL was empty).
Maybe that's something we can fix as well, to make CI builds on pull requests working :-)

@tuomas2
Copy link

tuomas2 commented Jan 18, 2024

The android logger slf4j implementation is probably already included transitively in some other dependencies of and-bible !

Good point. Then it would be better to put dependency explicitly. AndBible/and-bible#3174

Thank a lot for the merge !

By the way, I saw that the CI build on the pull request had failed (cf. https://github.com/AndBible/jsword/actions/runs/7560282128/job/20605220349 ), apparently because it didn't manage to retrieve the URL of the testmods file (the secrets.DOWNLOAD_TEST_MODULES_URL was empty). Maybe that's something we can fix as well, to make CI builds on pull requests working :-)

I noticed that too. I have a feeling it has worked in the past, but not any more for some reason. It works OK when running it for local branch (and I think also OK for local pull requests).

I can give you write access to repository (master and develop branches are protected) so you can create branches locally and make prs from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants