Remove duplicated tests that are no longer necessary #224
Conversation
This file contains 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
emdash-ie
approved these changes
Aug 27, 2024
There was a problem hiding this comment.
This looks good to me: shall we merge tomorrow?
I ran locally to make sure the tests still pass on the PR, and they do. Am I right in my impression that the tests are identical for 2.12 and 2.13, and if so is there any way to do that without requiring the duplicated files? Haven’t looked at tests for specific scala versions before.
|
I’ve been looking into this and doing some testing, and it seems that there’s no need to duplicate files: any files in Here’s the output from my testing, with warnings stripped out to make it clearer what’s going on: So I think we should put all the tests under |
|
Ok, did some looking through the history! The tests for 2.11 were introduced in #172. It’s a bit hard to see from the diff on github, but the newly-introduced 2.11 tests at that point were almost identical to the 2.12 tests, differing only in the diff-producing code they used (2.12 on the left, 2.11 on the right): Since the tests also used different versions of diffson-circe, my guess is that this is something that changed in that library between 3.1.1 and 4.0.0. The difference between the v3 documentation and the current documentation confirms that. 3.1.1 is the latest full version of diffson-circe_2.11, explaining why that version was used for the tests. On main the tests have drifted a bit further, because I and others updated only the 2.12 tests. Since we no longer support 2.11, I reckon we can go back to having just one version of the tests, using a v4 version of diffson-circe: I will take a look. I also don’t think we should merge this PR as-is just yet. (Side note: I hadn’t noticed the different dependencies for different scala versions: I will check that this currently makes sense, as I’m worried we may be testing against old fezziwig versions.) |
|
It looks like the current situation is just that the 2.11 tests haven’t been run since that version was removed from crossScalaVersions. There’s also no need for me to worry about not having updated the So, I think all we should do now is remove the 2.11 tests altogether, and make the 2.12 tests run at every scala version by moving them back into |
These tests were originally duplicated across two scala versions in [PR #172](#172), when new tests were added for scala 2.11. There’s not much info in the PR or commit history, but it looks like the reason for duplicating the source code was to use an older version of diffson-circe, where the API is slightly different (v3 is the last released version for 2.11, so the v4 version in use in the tests at the time couldn’t be used in the 2.11 tests). Since we now don’t support 2.11 anymore, the test source code doesn’t need to differ across different scala versions. (On main at the moment they differ slightly across the 2.12 and 2.11 tests, but only because recent changes have been made to 2.12 and not 2.11.) Since sbt runs tests that are not version-specific (e.g. those in `test/scala` rather than `test/scala-2.x`) for every requested scala version, there’s no need to duplicate the files when the tests are intended to be the same. As such, this commit makes the newer tests not version-specific anymore, by moving them back to json/src/test/scala, deleting the second copy.
emdash-ie
force-pushed
the
bt/fix-json-tests
branch
from
37f89fb to
be5cc7f
Compare
|
Since I’ve made changes, I’ll get someone else to review it rather than relying on my own earlier review. |
emdash-ie
changed the title
jonathonherbert
approved these changes
Aug 30, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
What does this change?
Support for Scala 2.11 was removed here: ab78878.
This pr adds tests for Scala 2.13 and removes 2.11 tests.EDIT: This PR now removes the 2.11 tests entirely, and makes the existing 2.12 tests run for all supported scala versions. This means the repository tests are now run for scala 2.13 as well as for scala 2.12. See messages below for more detail.
How to test
sbt "clean; test"on main with this branch: see that the former compiles source code but doesn’t run any tests, whereas the latter compiles code and runs testssbt "clean; +test"on main with this branch: on main the tests are only run for scala 2.12, but on this branch they’re also run for 2.13.