Enable integration tests #29

forus · 2024-03-19T16:14:06Z

Fix and un-ignore most of tests that work with cbioportal database (aka integration tests)
Document how to prepare cbioportal test database and run integration tests

inodb · 2024-03-25T14:56:41Z

@forus it looks like the Java CI is failing - is that expected?

Error: HTTP Status 403 for request POST https://api.github.com/repos/cBioPortal/cbioportal-core/dependency-graph/snapshots
Error: Response body:
{
"message": "Resource not accessible by integration",
"documentation_url": "https://docs.github.com/rest/dependency-graph/dependency-submission#create-a-snapshot-of-dependencies-for-a-repository"
}
Error: Resource not accessible by integration
Error: HttpError: Resource not accessible by integration
at /home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@octokit/request/dist-node/index.js:124:1
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at L (/home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@github/dependency-submission-toolkit/dist/index.js:6:1)

/home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@github/dependency-submission-toolkit/dist/index.js:7
${JSON.stringify(n.response.data,void 0,2)})),n instanceof Error&&(a.error(n.message),n.stack&&a.error(n.stack)),new Error(Failed to submit snapshot: ${n}`)}}
^
Error: Failed to submit snapshot: HttpError: Resource not accessible by integration
at L (/home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@github/dependency-submission-toolkit/dist/index.js:7:1)
at processTicksAndRejections (node:internal/process/task_queues:95:5)

forus · 2024-03-27T15:27:43Z

@forus it looks like the Java CI is failing - is that expected?

Error: HTTP Status 403 for request POST https://api.github.com/repos/cBioPortal/cbioportal-core/dependency-graph/snapshots
Error: Response body:
{
"message": "Resource not accessible by integration",
"documentation_url": "https://docs.github.com/rest/dependency-graph/dependency-submission#create-a-snapshot-of-dependencies-for-a-repository"
}
Error: Resource not accessible by integration
Error: HttpError: Resource not accessible by integration
at /home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@octokit/request/dist-node/index.js:124:1
at processTicksAndRejections (node:internal/process/task_queues:95:5)
at L (/home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@github/dependency-submission-toolkit/dist/index.js:6:1)

/home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@github/dependency-submission-toolkit/dist/index.js:7
${JSON.stringify(n.response.data,void 0,2)})),n instanceof Error&&(a.error(n.message),n.stack&&a.error(n.stack)),new Error(Failed to submit snapshot: ${n}`)}}
^
Error: Failed to submit snapshot: HttpError: Resource not accessible by integration
at L (/home/runner/work/_actions/advanced-security/maven-dependency-submission-action/v4.0.0/node_modules/@github/dependency-submission-toolkit/dist/index.js:7:1)
at processTicksAndRejections (node:internal/process/task_queues:95:5)

@inodb Hi! No, that is not expected. I'm looking into it.

forus · 2024-03-27T15:45:48Z

@inodb It seems like a permission problem. The dependency graph snapshot could not be uploaded to GitHub.

Cold it be because the PR is created between organisations? se4bio -> cBioPortal

@sheridancbio gotten this issue as well #25

haynescd

lgtm

haynescd · 2024-03-29T15:02:09Z

@inodb I think we should update the Java CI with maven job to only update dep. graph on push to main not PR

JREastonMarks

Instead of IT and not IT names in front of the classes you should divide them into two directories it and <>. This will make the file names look nicer, show better organization inside the IDE, and help out with the way that the POM is setup.

Other then that this LGTM.

forus · 2024-03-29T19:22:16Z

Instead of IT and not IT names in front of the classes you should divide them into two directories it and <>. This will make the file names look nicer, show better organization inside the IDE, and help out with the way that the POM is setup.

Other then that this LGTM.

Hi Jeremy (@JREastonMarks),

Thank you for your feedback. I agree with you that it would be a better code organisation.

In fact, I tried to split directories to src/test/java and src/integration-test/java at the beginning. Unfortunately, I encountered challenges in getting this setup to function as intended.

In my most recent attempt, I used add-test-source and add-test-resource goals of build-helper-maven-plugin plugin to get integration tests to compile. However the plugin makes unit tests and integration tests to compile to the very same class directory. I did not find how can you compile them in separate folders. Consequently, it remains necessary to differentiate between unit tests and integration tests by naming convention, specifically excluding integration tests (**/IT*) during the test phase and including them in the integration-test phase. Otherwise, test phase would pick up running integration tests.
I found it even more confusing to both place integration tests in a separate folder and ensure their names match the pattern.

Below is the relevant plugin configuration used:

<plugin>
				<groupId>org.apache.maven.plugins</groupId>
				<artifactId>maven-surefire-plugin</artifactId>
				<version>2.21.0</version>
				<executions>
					<execution>
						<id>default-test</id>
						<phase>test</phase>
						<goals>
							<goal>test</goal>
						</goals>
						<configuration>
							<excludes>
								<exclude>**/IT*</exclude>
							</excludes>
						</configuration>
					</execution>
					<execution>
						<id>integration-test</id>
						<phase>integration-test</phase>
						<goals>
							<goal>test</goal>
						</goals>
						<configuration>
							<includes>
								<include>**/IT*</include>
							</includes>
							<forkCount>1</forkCount>
							<reuseForks>false</reuseForks>
						</configuration>
					</execution>
				</executions>
			</plugin>

	<plugin>
				<groupId>org.codehaus.mojo</groupId>
				<artifactId>build-helper-maven-plugin</artifactId>
				<version>3.2.0</version>
				<executions>
					<execution>
						<id>add-integration-test-sources</id>
						<phase>generate-test-sources</phase>
						<goals>
							<goal>add-test-source</goal>
						</goals>
						<configuration>
							<sources>
								<source>src/integration-test/java</source>
							</sources>
						</configuration>
					</execution>
					<execution>
						<id>add-integration-test-resources</id>
						<phase>generate-test-resources</phase>
						<goals>
							<goal>add-test-resource</goal>
						</goals>
						<configuration>
							<resources>
								<resource>
									<directory>src/integration-test/resources</directory>
								</resource>
							</resources>
						</configuration>
					</execution>
				</executions>
			</plugin>

Have you, or anyone else on the team, successfully implemented such a directory split before? If so, I'd greatly appreciate any insights or suggestions on how to resolve the issues encountered.

Given the improvements this PR brings to our current setup, I recommend proceeding with the merge. We can revisit and attempt a more distinct separation of test directories when we know more.

JREastonMarks · 2024-03-29T19:35:16Z

In fact, I tried to split directories to src/test/java and src/integration-test/java at the beginning. Unfortunately, I encountered challenges in getting this setup to function as intended.

I would do something like the following:
src/test/java/org/mskcc/cbio/portal/integrationTest/pancancer/PanCancerStudySummaryImport.java

That way you aren't messing with the file organization which can make build systems unhappy.

forus · 2024-03-29T19:51:19Z

In fact, I tried to split directories to src/test/java and src/integration-test/java at the beginning. Unfortunately, I encountered challenges in getting this setup to function as intended.

I would do something like the following: src/test/java/org/mskcc/cbio/portal/integrationTest/pancancer/PanCancerStudySummaryImport.java

That way you aren't messing with the file organization which can make build systems unhappy.

@JREastonMarks

I see. So you propose to have a special integrationTest java package. That could work.

How about having src/test/java/integrationTest/org/mskcc/cbio/portal/pancancer/TestPanCancerStudySummaryImport.java instead or maybe even something like src/test/java/integrationTest/pancancer/TestPanCancerStudySummaryImport.java ?

I am not sure you could drop Test* in the name. We have helper classes that are not test classes e.g. org.mskcc.cbio.portal.pancancer.LargeTestSetGenerator

JREastonMarks · 2024-04-01T12:40:13Z

@forus The issue with doing at such a high level is that /src/test/[PACKAGE NAME] is that your code is in the IDE is going to be in multiple places as well as issues getting it to run. If you keep the code closer to the packages and areas that it tests and touches then it makes it easier for developers to work with it. So instead of src/test/java/integrationTest/org/mskcc/cbio/portal/pancancer/TestPanCancerStudySummaryImport.java I would recommend something like this src/test/java/org/mskcc/cbio/portal/pancancer/integrationTest/TestPanCancerStudySummaryImport.java or src/test/java/org/mskcc/cbio/portal/integrationTest/pancancer/TestPanCancerStudySummaryImport.java .

The big goal, outside of just having the tests run, is to make sure that they can easily be updated and accessed. We can write amazing tests but if people don't keep up with them or have a difficult time running them then they lose their purpose.

forus · 2024-04-01T15:30:05Z

@JREastonMarks Please have another look. Test classes names do not to follow any naming convention to run properly after all. The surefire plugin distinguish tests from utility test classes just fine and we don't need to help it in this.
However we should follow a naming convention to make distinguishing test classes from production classes (testee) more easier. I renamed integration test: IT* -> Test* for readability purpose.

JREastonMarks

It's closer but still some changes are needed

src/test/java/org/mskcc/cbio/portal/integrationTest/dao/TestDaoSampleList.java

src/test/java/org/mskcc/cbio/portal/integrationTest/dao/TestDaoSampleProfile.java

src/test/java/org/mskcc/cbio/portal/integrationTest/pancancer/LargeTestSetGenerator.java

...st/java/org/mskcc/cbio/portal/integrationTest/pancancer/TestPanCancerStudySummaryImport.java

src/test/java/org/mskcc/cbio/portal/integrationTest/scripts/TestImportTabDelimData.java

src/test/java/org/mskcc/cbio/portal/integrationTest/servlet/TestWebService.java

Warning was not shown because process mistakenly thought it's running from a web server

They passing when run one by one. But fail when you run them in batch due dirty database state (created by other tests?).

With the old version, db integration tests did not roll back inserted data properly

Integrationt test class name does not have to follow any naming convention anymore as soon as it placed under integrationTest package. We renamed all integration tests from IT* to Test* for readability purpose.

src/test/java/org/mskcc/cbio/portal/scripts/TestCutInvalidCases.java

src/test/java/org/mskcc/cbio/portal/integrationTest/dao/TestDaoCancerStudy.java

JREastonMarks

One small thing left but it looks good to me

src/test/java/org/mskcc/cbio/portal/integrationTest/dao/TestDaoMutation.java

forus · 2024-04-05T18:45:57Z

After rebasing on the recent version of main branch, integration tests started to fail. I am looking into that.

forus · 2024-04-08T19:09:49Z

After rebasing on the recent version of main branch, integration tests started to fail. I am looking into that.

I've commented out the configuration that caused integration tests to fail

forus force-pushed the enable-integration-tests branch from 3191138 to b07232a Compare March 21, 2024 15:11

haynescd requested review from JREastonMarks and haynescd March 28, 2024 18:08

forus mentioned this pull request Mar 29, 2024

Dockerise #16

Merged

haynescd approved these changes Mar 29, 2024

View reviewed changes

JREastonMarks suggested changes Mar 29, 2024

View reviewed changes

forus requested a review from JREastonMarks March 29, 2024 19:29

JREastonMarks suggested changes Apr 3, 2024

View reviewed changes

forus requested a review from JREastonMarks April 3, 2024 15:42

ruslan-forostianov added 10 commits April 4, 2024 16:04

Make import clinical data tests to run

6b4dceb

Fix testImportClinicalData_WithDuplInMixedAttrFormat

3e6f92a

Warning was not shown because process mistakenly thought it's running from a web server

Enable TestImportProfileData

3632d2b

Enable the rest of passing script tests

0ce5321

They passing when run one by one. But fail when you run them in batch due dirty database state (created by other tests?).

Upgrade surefire version to insulate test executions better

8b4f29c

With the old version, db integration tests did not roll back inserted data properly

Run db tests at integration-test step

29ab2da

Un-ignore the rest of passing integration tests

0de4217

Document how to run integration tests

9816ecf

Rename and move int.tests to integerationTest package

49b5f4b

Integrationt test class name does not have to follow any naming convention anymore as soon as it placed under integrationTest package. We renamed all integration tests from IT* to Test* for readability purpose.

Replace wildcard imports in integrationTest folder

0b48914

forus force-pushed the enable-integration-tests branch from 04d49aa to 0b48914 Compare April 4, 2024 14:04

Move TestImportGenesetData back to own class

8e49ff5

forus force-pushed the enable-integration-tests branch from 07906b8 to 8e49ff5 Compare April 4, 2024 19:01

JREastonMarks reviewed Apr 4, 2024

View reviewed changes

src/test/java/org/mskcc/cbio/portal/scripts/TestCutInvalidCases.java Outdated Show resolved Hide resolved

src/test/java/org/mskcc/cbio/portal/integrationTest/dao/TestDaoCancerStudy.java Outdated Show resolved Hide resolved

Use static imports for asserts

257d2a5

forus requested a review from JREastonMarks April 5, 2024 07:22

JREastonMarks approved these changes Apr 5, 2024

View reviewed changes

src/test/java/org/mskcc/cbio/portal/integrationTest/dao/TestDaoMutation.java Outdated Show resolved Hide resolved

ruslan-forostianov added 2 commits April 6, 2024 08:00

Use asserts from org.junit.Assert class everywhere

b6ca35d

Comment application context configuration that caused tests to fail

63d58d6

haynescd merged commit b83821c into cBioPortal:main Apr 8, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable integration tests #29

Enable integration tests #29

forus commented Mar 19, 2024

inodb commented Mar 25, 2024

forus commented Mar 27, 2024

forus commented Mar 27, 2024

haynescd left a comment

haynescd commented Mar 29, 2024

JREastonMarks left a comment

forus commented Mar 29, 2024 •

edited

Loading

JREastonMarks commented Mar 29, 2024

forus commented Mar 29, 2024 •

edited

Loading

JREastonMarks commented Apr 1, 2024

forus commented Apr 1, 2024

JREastonMarks left a comment

JREastonMarks left a comment

forus commented Apr 5, 2024

forus commented Apr 8, 2024

Enable integration tests #29

Enable integration tests #29

Conversation

forus commented Mar 19, 2024

inodb commented Mar 25, 2024

forus commented Mar 27, 2024

forus commented Mar 27, 2024

haynescd left a comment

Choose a reason for hiding this comment

haynescd commented Mar 29, 2024

JREastonMarks left a comment

Choose a reason for hiding this comment

forus commented Mar 29, 2024 • edited Loading

JREastonMarks commented Mar 29, 2024

forus commented Mar 29, 2024 • edited Loading

JREastonMarks commented Apr 1, 2024

forus commented Apr 1, 2024

JREastonMarks left a comment

Choose a reason for hiding this comment

JREastonMarks left a comment

Choose a reason for hiding this comment

forus commented Apr 5, 2024

forus commented Apr 8, 2024

forus commented Mar 29, 2024 •

edited

Loading

forus commented Mar 29, 2024 •

edited

Loading