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

Re-enable tests in CI (Eclipse Jenkins) #1166

Merged
merged 21 commits into from Jan 13, 2021
Merged

Conversation

javierron
Copy link
Contributor

I'm hijacking @andre15silva's PR #1164 to attempt some fixes myself. Any progress will be rebased to his PR.

andre15silva and others added 20 commits December 22, 2020 10:52
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: Javier Ron <javierron90@gmail.com>
@andre15silva
Copy link
Contributor

Just to keep you up to date then, the current problems are:

  1. The pipeline stage works on the local docker image, but on the Jenkins CI it gives those errors.
  2. maven-repair has a problem, when running locally in the docker container, with spoon-core jar file not being available in the local repository. I'm trying to reproduce that in Jenkins.
  3. Yesterday maven-repair hit an infinite loop. I'm not sure what is causing it. I'm adding a timeout so that doesn't happen again.

@javierron
Copy link
Contributor Author

Just to keep you up to date then, the current problems are:

1. The `pipeline` stage works on the local docker image, but on the Jenkins CI it gives those errors.

2. `maven-repair` has a problem, when running locally in the docker container, with `spoon-core` jar file not being available in the local repository. I'm trying to reproduce that in Jenkins.

3. Yesterday `maven-repair` hit an infinite loop. I'm not sure what is causing it. I'm adding a timeout so that doesn't happen again.

Cool! thanks for the info! I am focusing on point 3 for now. I believe the problem might be here:

My guess is that the mvn clean test command is failing and the tests waits forever. I'll keep you updated.

@andre15silva
Copy link
Contributor

Could be. When running on the local docker container the only problem is with the Nopol test not finding the spoon-core jar, so should be related to the environment in Jenkins.

@javierron javierron force-pushed the ci-fix branch 7 times, most recently from f189028 to 7b9bc08 Compare January 4, 2021 15:54
@javierron javierron force-pushed the ci-fix branch 12 times, most recently from 9a180a5 to f001b0d Compare January 4, 2021 20:19
@javierron
Copy link
Contributor Author

javierron commented Jan 4, 2021

Solved 2 & 3 (log). I will attempt point 1 tomorrow and hopefully the modification on the Jenkinsfile will fix some of the problems of the pipeline. @andre15silva

@javierron javierron force-pushed the ci-fix branch 2 times, most recently from c85daf2 to 91977a1 Compare January 5, 2021 12:11
Add explicit dependecies on maven-reapiar pom to avoid conflicts and missing jars on tests

Signed-off-by: Javier Ron <javierron90@gmail.com>
@javierron
Copy link
Contributor Author

This PR is passing the tests now.
@andre15silva are there any new changes on your branch? If not, we can skip the rebase and merge this PR instead.

Comment on lines +91 to 93
container('maven') {
sh 'bash ./.ci/ci-maven-repair.sh'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really a need to use a separate script for maven-repair? It would be nice to merge this with run-with-core. The only difference is the NPEFix version option.

If we change this we can eliminate xmlstarlet from the Docker image too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure of what does that version property does, have you tried to run the tests without it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and they work. I also don't know why that is needed. I haven't found any usage of a property NPEFIX_VERSION.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +149 to +163
<dependency>
<groupId>fr.inria.gforge.spoon</groupId>
<artifactId>spoon-core</artifactId>
<version>7.0.0</version>
</dependency>
<dependency>
<groupId>commons-cli</groupId>
<artifactId>commons-cli</artifactId>
<version>1.3</version>
</dependency>
<dependency>
<groupId>org.json</groupId>
<artifactId>json</artifactId>
<version>20160810</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the best approach, albeit it works. Shouldn't the dependencies be fetched transitively?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this is the best approach

I agree with you. At least in the case of spoon-core I could see that the dependency metadata (pom and sha files) is downloaded, but unless I put these here explicitly it does not download the jar. I assumed the same for commons-cli and json.

Also, I checked and the maven-repair tests work locally in my pc only because those jars were already there as a result of compiling another module.

If there is a better way to trigger the jar download, we can replace this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's very weird behavior indeed. While debugging I also saw that pom and sha files were downloaded, just not the jar file.

I think it's important to get this right tho, because having sub-dependencies hardcoded makes it hard to maintain.

I will look further into it after my exam tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

mvn dependency:tree for maven-repair is in https://gist.github.com/andre15silva/254c0de8ef2d2a7ac45c834f404119c4

The issue is that these three dependencies are being omitted because of conflicts with sub-dependencies from other repair tools that use these libraries but with a different version.

[INFO]    +- (fr.inria.gforge.spoon:spoon-core:jar:7.0.0:compile - omitted for conflict with 6.0.0)
[INFO]    +- (commons-cli:commons-cli:jar:1.3:compile - omitted for conflict with 1.2)
[INFO]    \- (org.json:json:jar:20160810:compile - omitted for conflict with 20160212)

If we added the Nopol dependency first, it would fetch these versions. It's also not a very elegant solution tho. Yours is probably better, as it is explicitly written.

Another option would be to change getClassPathFromPom to take into account that the version explicited in the repair tools pom files may not be available locally and that another version has to be used. I'm not sure this is a very good idea tho.

@javierron WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we added the Nopol dependency first, it would fetch these versions. It's also not a very elegant solution tho. Yours is probably better, as it is explicitly written.

I believe I tried this, and it did not resolve the conflicts.

Another option would be to change getClassPathFromPom to take into account that the version explicited in the repair tools pom files may not be available locally and that another version has to be used.

I don't think this is a good idea either, we may create an even more complicated compatibility/dependency issue.

@andre15silva
Copy link
Contributor

This PR is passing the tests now.
@andre15silva are there any new changes on your branch? If not, we can skip the rebase and merge this PR instead.

The only changes were the timeouts, but you have already included that here. I added some comments, tell me what you think.

Other than that, we can skip the rebase and merge this yes.

@javierron javierron marked this pull request as ready for review January 6, 2021 14:44
@monperrus
Copy link
Contributor

Thanks a lot for the heavy work. LGTM to merge and proceed. We can fine-tune later.

OK for you?

@javierron
Copy link
Contributor Author

OK for you?

OK for me

@andre15silva
Copy link
Contributor

andre15silva commented Jan 13, 2021

OK for you?

OK for me

OK for me too

@monperrus monperrus changed the title Enable tests in CI Re-enable tests in CI (Eclipse Jenkins) Jan 13, 2021
@monperrus monperrus merged commit 7e4637a into eclipse:master Jan 13, 2021
@monperrus
Copy link
Contributor

Thanks a lot for the foundational PR

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

3 participants