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

refactor: JUnit Jupiter migration from JUnit 4.x #998

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

aamotharald
Copy link

@aamotharald
Copy link
Author

Hi @artem-smotrakov ,
I am working on porting your OS project from JUnit4 to JUnit5.

There are 3 things I would like to discuss with you:

  1. setting the locale en_US for formatting functionality
  2. How can I automatically fix the checkstyle violations with regards to code formatting? Do you have a fpormatting tool / plugin?
  3. I see a Unit test failing that passed on the JUnit4 framwrok. the Failing test is ScoreVerificationTest.testVerification . I would appreciate some consulting.

Have a great one and happy FOSSTARS rating!

@artem-smotrakov
Copy link
Collaborator

Hey @aamotharald !

I am working on porting your OS project from JUnit4 to JUnit5.

I have not been really involved into this project for a long time, maybe @Sachpat or @sourabhsparkala can help better.

In general, I'd say do whatever it needs to move it to JUnit 5 :)

setting the locale en_US for formatting functionality

I think this should be fine, I don't see any issue here.

How can I automatically fix the checkstyle violations with regards to code formatting? Do you have a fpormatting tool / plugin?

It depends on your IDE. The project uses Google Java Style Guide

https://github.com/SAP/fosstars-rating-core/blob/master/CONTRIBUTING.md#code-formatting

Depending on IDE you use, I'd guess there should be plugins that supports that.

I see a Unit test failing that passed on the JUnit4 framwrok. the Failing test is ScoreVerificationTest.testVerification . I would appreciate some consulting.

What is the error?

@aamotharald
Copy link
Author

HI @artem-smotrakov ,

the test failure is:

Verify SecurityReviewScore with 4 test vectors
[+] Test vector #2 failed
[+]     reason: Expected a score in the interval [9.5, 10.0] but 5.0 returned
[+]     alias:  fresh_review
[+] Test vector #3 failed
[+]     reason: Expected a score in the interval [2.0, 5.0] but 1.6666666666666667 returned
[+]     alias:  old_review

and

Verify VulnerabilityDiscoveryAndSecurityTestingScore with 8 test vectors
[+] Test vector #4 failed
[+]     reason: Expected a score in the interval [0.0, 1.0] but 2.0 returned
[+]     alias:  bad testing and recent vulnerabilities
[+] Test vector #6 failed
[+]     reason: Expected a score in the interval [7.0, 9.0] but 10.0 returned
[+]     alias:  good testing and recent vulnerabilities

As no logic got changed this is quite cumbersome to me.
I just changed the testing framework and then suddenly scenarios start to fail.
I am trying to understand the yamls and what's tested there.

@aamotharald
Copy link
Author

I can also see 2 failing integration tests which I have to investigate.

@aamotharald
Copy link
Author

Found the problem.

PR is ready to merge - well but in 1 year tests will fail again as then date distances for the scores have changed.

@aamotharald
Copy link
Author

If the CLI tests fail, the reason will be a timeout from the NVD database api.

Who can add a secret for this pipeline?
image

Besides that the tool is now "Modernized" ..

@aamotharald
Copy link
Author

OK, I will now pimp the 3 Dockerfiles in this project and simplify them.
just showing the green integration tests as they might be flaky without an NVD database access token..
image

Copy link

@skateball skateball left a comment

Choose a reason for hiding this comment

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

nice, is replacing my PR #1000

- name: Set up JDK 1.8
uses: actions/setup-java@v3
- name: Checkout repository
uses: actions/[email protected]

Choose a reason for hiding this comment

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

Suggested change
uses: actions/checkout@v3.5.3
uses: actions/checkout@v4

Choose a reason for hiding this comment

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

newer version available

@@ -23,9 +23,15 @@ jobs:
- name: Checkout repository
uses: actions/[email protected]

Choose a reason for hiding this comment

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

Suggested change
uses: actions/checkout@v3.5.3
uses: actions/checkout@v4

Choose a reason for hiding this comment

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

newer version available

@@ -18,11 +18,11 @@ jobs:
os: [ubuntu-latest, windows-latest, macos-latest]
steps:
- uses: actions/[email protected]

Choose a reason for hiding this comment

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

Suggested change
- uses: actions/checkout@v3.5.3
- uses: actions/checkout@v4

Choose a reason for hiding this comment

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

newer version available

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.

3 participants