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

Clean up compliance tests and migrate to more unit tests #1236

Closed
jeenbroekstra opened this issue Jan 7, 2019 · 11 comments

Comments

@jeenbroekstra
Copy link
Contributor

commented Jan 7, 2019

Several test implementations in the various compliance modules are overlapping in coverage, and/or really should be refactored to proper unit tests (in src/test/java of the module/class being tested, instead of a separate compliance module).

A difficulty is that any test that needs a sail back-end store can introduce a circular dependency in maven when added to it main module instead of compliance. We should look into a mocking/stub setup to help alleviate this.

Followup of #1230

@jeenbroekstra jeenbroekstra added this to the 2.5.0 milestone Jan 7, 2019

@jeenbroekstra jeenbroekstra added this to To do in 2.5.0 development via automation Jan 7, 2019

@jeenbroekstra jeenbroekstra modified the milestones: 2.5.0, 3.0 Mar 7, 2019

@jeenbroekstra jeenbroekstra removed this from To do in 2.5.0 development Mar 7, 2019

@jeenbroekstra jeenbroekstra added this to To do in Next release via automation Mar 7, 2019

@jeenbroekstra

This comment has been minimized.

@jeenbroekstra jeenbroekstra self-assigned this Mar 17, 2019

@jeenbroekstra jeenbroekstra moved this from To do to In progress in Next release Mar 17, 2019

@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

For the Model API, I will get rid of the compliance module and the testsuite completely, and just turn the whole thing into unit tests. While we're at it we'll replace the poorly integrated code using Apache commons collections tests with Google Guava test code for Java collections.

CQ for the guava testlib logged here : https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19292

jeenbroekstra added a commit that referenced this issue Mar 17, 2019
jeenbroekstra added a commit that referenced this issue Mar 17, 2019
@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

CQ approved

@hmottestad

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

I've had some issues with debugging the SHACL compliance tests in IntelliJ. Will this change make it possible to debug a single test?

@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

@hmottestad I'm not immediately sure. What you describe is often caused by using tests that are encoded in resource files - in such cases the unit test runner only knows at runtime which tests exist, so it can't "zoom in" on a single test easily. I'm not proposing to refactor that complete setup as part of this cleanup as well (though that doesn't mean I'm against doing it, just that I want to keep this issue manageable in size ;)).

jeenbroekstra added a commit that referenced this issue Mar 22, 2019
Merge pull request #1349 from eclipse/issues/GH-1236-testsuite-refactor
GH-1236 removed model compliance module and extended rdf4j-model unit test suite
jeenbroekstra added a commit to eclipse/rdf4j-testsuite that referenced this issue Mar 24, 2019
jeenbroekstra added a commit that referenced this issue Mar 24, 2019
@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

The proposed new setup is as follows:

  1. all compliance tests will be moved into the rdf4j-testsuite repo. This removes circular dependencies between the other repos and the testsuites, and in addition makes quicker test feedback loops possible on the compliance tests.
  2. various tests that were in the past designed as compliance are, in fact, unit tests, and will be moved from the testsuites/compliance modules to their respective functional modules. For example, the model tests, and the queryresultio tests.

A downside of this approach is that compliance/integration testing will not be run as part of the verification cycle for rdf4j, rdf4j-storage, and rdf4j-tools. It will instead be part of the rdf4j-testsuite compile/verify cycle. In order to mitigate this it's important that we migrate as many tests as possible to the level of unit tests (without reintroducing cyclic dependencies, however).

jeenbroekstra added a commit that referenced this issue Mar 24, 2019
jeenbroekstra added a commit to eclipse/rdf4j-testsuite that referenced this issue Mar 24, 2019
@hmottestad

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

Are you proposing that we don’t even call the compliance tests in the respective repos? Today, in RDF4J-storage, there is a compliance module which extends a bunch of tests.

I’m very worried if all the tests aren’t run automatically on PRs in Jenkins!

I propose the following:

  • all tests that are not shared by more than one repo are moved into that repo (compliance or not)
  • all modules that cause cyclic dependencies are moved into single repos, and possibly merged into single modules

I must admit, that after we split up RDF4J into multiple repos, I’ve become a big fan of single repos. For build performance we could instead try out gradle.

Also, I find it very annoying that when a compliance test fails, I need to go to the testsuite github and search up the file in resources that was loaded as part of the test.

@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

A possible change is to move the test suites and the compliance tests into the corresponding repos, and get rid of rdf4j-testsuites altogether. I didn't immediately want to go that radical but perhaps we should.

@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

What I hadn't thought of when I proposed that is that we can't do that. The testsuites and compliance tests all have multiple dependencies on modules in both rdf4j and rdf4j-storage (and even rdf4j-tools).

For example, the rio-testsuite tests use the memory sail. This means we can't compile and execute them until after the memory sail module has been built. Therefore we can not put these tests in the rdf4j repo, because that repo gets built before the rdf4j-storage repo (which contains the memory sail). This is precisely why we currently have this convoluted setup with only including the compliance modules in the maven build when executing tests, and having jenkins first do a deployment without tests and only afterwards running the compliance tests.

As a first pass I will continue with my earlier proposal: move all compliance tests to the testsuite, and migrate as many non-complex tests as possible back to proper unit tests included in each functional module itself.

But I'm considering further changes. One possible change is to make sure that dependencies on other modules in the testsuite are not against the latest snapshot, but against a fixed release - the thinking being that it shouldn't matter for, for example, the rio testsuite which particular version of the memory sail it uses - it just needs a store to keep some data in. If we do that, we can have these tests moved back into the rdf4j repo, because they no longer rely on the latest rdf4j-storage repo being built.

jeenbroekstra added a commit that referenced this issue Mar 30, 2019

@jeenbroekstra jeenbroekstra reopened this Apr 7, 2019

jeenbroekstra added a commit that referenced this issue Apr 7, 2019
jeenbroekstra added a commit that referenced this issue Apr 7, 2019
Merge pull request #1386 from eclipse/issues/GH-1236-testsuite-refactor
GH-1236 include minimally-conforming query result formats for compliance

@jeenbroekstra jeenbroekstra moved this from In progress to Done in Next release Apr 27, 2019

reckart added a commit to inception-project/rdf4j that referenced this issue May 19, 2019
Merge branch 'master' into inception-2.5.x
* master:
  eclipseGH-1403 wildcard processor skips binds and filter constraints
  eclipseGH-1236 removed model compliance module and extended rdf4j-model unit test suite instead
  fixed merge conflict residual
  fixed formatting
  Update Jackson dependency
  formatter no longer joins lines back up if deliberately split
  eclipseGH-1240 eclipseGH-822 updated readme and contributing guidelines
  eclipseGH-1240 removed redundant entries from xml file
  eclipseGH-822 formatter should ignore html (for now)
  eclipseGH-822 check if non-empty maven.config makes a difference for Jenkins [WIP]
  eclipseGH-1240 eclipseGH-822 enforce unix line endings
  eclipseGH-1240 formatted with eclipse conventions
  eclipseGH-822 configured formatter plugin with new code conventions
  eclipse#1321 - "file:" not a recognized protocol in SparqlBuilder
  Added tests for org.eclipse.rdf4j.common.io.ByteArrayUtil These tests were written using Diffblue Cover.
  eclipse#1321 - "file:" not a recognized protocol in SparqlBuilder

% Conflicts:
%	compliance/model/pom.xml
%	sparqlbuilder/src/main/java/org/eclipse/rdf4j/sparqlbuilder/core/query/OuterQuery.java
%	sparqlbuilder/src/main/java/org/eclipse/rdf4j/sparqlbuilder/rdf/Rdf.java

@jeenbroekstra jeenbroekstra reopened this Jun 23, 2019

Next release automation moved this from Done to In progress Jun 23, 2019

@jeenbroekstra

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Turns out I missed a few circularities in test dependencies. Unfortunately this is blocking the first milestone. Re-opening to clean up.

jeenbroekstra added a commit that referenced this issue Jun 23, 2019
GH-1236 split store-testsuite
into repository-testsuite and sail-testsuite
jeenbroekstra added a commit to eclipse/rdf4j-storage that referenced this issue Jun 23, 2019
eclipse/rdf4j#1236 split store-testsuite
into repository-testsuite and sail-testsuite
jeenbroekstra added a commit that referenced this issue Jun 27, 2019
jeenbroekstra added a commit that referenced this issue Jun 27, 2019
jeenbroekstra added a commit to eclipse/rdf4j-storage that referenced this issue Jun 27, 2019
jeenbroekstra added a commit to eclipse/rdf4j-storage that referenced this issue Jun 27, 2019
jeenbroekstra added a commit to eclipse/rdf4j-storage that referenced this issue Jun 27, 2019
jeenbroekstra added a commit to eclipse/rdf4j-storage that referenced this issue Jun 27, 2019
jeenbroekstra added a commit that referenced this issue Jun 27, 2019
jeenbroekstra added a commit that referenced this issue Jun 29, 2019

Next release automation moved this from In progress to Done Jul 28, 2019

jeenbroekstra added a commit that referenced this issue Aug 22, 2019
jeenbroekstra added a commit that referenced this issue Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.