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

test(reporters): Add three scan issues to report test assets #8552

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

fviernau
Copy link
Member

First preparations for implementing #8373.

@fviernau fviernau requested a review from a team as a code owner April 22, 2024 13:56
@fviernau fviernau force-pushed the webapp-filter-excluded-preparations branch from e01a939 to 50aeb43 Compare April 22, 2024 14:02
@fviernau fviernau enabled auto-merge (rebase) April 22, 2024 14:06
@@ -65,59 +65,80 @@ issue_resolutions:
comment: "A comment explaining why the issue can be ignored."
issues:
- _id: 0
timestamp: "2024-04-22T10:36:10.661544294Z"
Copy link
Member

Choose a reason for hiding this comment

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

Add one issue to an included- and another to an excluded project

"Add one issue to an included and another one to an excluded project"

This prepares adding the ability

"This prepares for adding the ability" or "This prepares to add the ability"

@fviernau fviernau force-pushed the webapp-filter-excluded-preparations branch from 50aeb43 to e89a519 Compare April 22, 2024 14:38
@fviernau fviernau requested a review from sschuberth April 22, 2024 14:38
@fviernau fviernau force-pushed the webapp-filter-excluded-preparations branch from e89a519 to 6b893b2 Compare April 22, 2024 14:43
@fviernau fviernau disabled auto-merge April 23, 2024 08:15
@fviernau fviernau marked this pull request as draft April 23, 2024 09:42
The replacement has been introduced for replacing the report creation
timestamp only. However, `replace()` replaces all matches and the
pattern would all so match the timestamp value of issues. So, replace
only the first match, so that issues (with timestamps) can be added in a
following change.

[1]: ba0e4cb

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the webapp-filter-excluded-preparations branch from 6b893b2 to 03dd6ae Compare April 24, 2024 08:57
@fviernau fviernau marked this pull request as ready for review April 24, 2024 08:57
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.68%. Comparing base (bee7613) to head (5ff7c71).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8552   +/-   ##
=========================================
  Coverage     67.68%   67.68%           
  Complexity     1006     1006           
=========================================
  Files           246      246           
  Lines          7924     7924           
  Branches        883      883           
=========================================
  Hits           5363     5363           
  Misses         2181     2181           
  Partials        380      380           
Flag Coverage Δ
funTest-docker 65.35% <ø> (ø)
funTest-non-docker 34.71% <ø> (ø)
test 37.55% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add one issue with an included and one issue with an excluded affected
path to an included project. Furthermore, add on issue with an included
affected path to an excluded project. This prepares to add the ability
to vizualize issues with excluded effected path as excluded, in the
WebApp as well as in the static HTML report.

Note: There are three copies of `reporter-test-input.yml` which are all
      updated for consistency. Furthermore, the affected paths of the
      issues have been choosen to be within the VCS path, so that the
      issues do net get filtered out by
      `OrtResult.getScanResultsForId()`.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the webapp-filter-excluded-preparations branch from 03dd6ae to 5ff7c71 Compare April 24, 2024 09:21
@@ -190,6 +221,97 @@ scan_results:
package_verification_code: "da39a3ee5e6b4b0d3255bfef95601890afd80709"
packages:
- _id: 0
id: "Gradle:org.ossreviewtoolkit:nested-fake-project:1.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Any idea why the numeric IDs of the packages seem to be swapped now? That makes the diff a bit difficult to read...

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. This is always imho difficult to figure out as the IDs are assigned by Jackson.

Copy link
Member Author

@fviernau fviernau Apr 24, 2024

Choose a reason for hiding this comment

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

@sschuberth - I've debugged this for quite a bit, and it seems to just depend on the order things get serialized. E.g. the first identifier which gets serialized gets the 0 assigned. The next identifier the 1 and so forth. So, with this change Gradle:org.ossreviewtoolkit:nested-fake-project:1.0.0 gets serialized first.

I believe such diffs we have to live with, as long as we use the above mentioned approach to assign identifiers.

Copy link
Member

Choose a reason for hiding this comment

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

E.g. the first identifier which gets serialized gets the 0 assigned.

Right, but why does the order of identifiers change with your PR, shouldn't it all be deterministic? In any case, thanks for looking into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, but why does the order of identifiers change with your PR, shouldn't it all be deterministic?

Hm, each time one make a small change to the test asset one needs to debug why jackson assigns the IDs in the way it does is imo raising the bar for contributions quite a bit. Can we shortcut this and live without the explanation ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Investigating further. Maybe @mnonnenmacher does the issue here ring any bell? (can you help?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it is as follows:

  1. The EvaluatedModel.issues property comes before EvaluatedModel.packages.
  2. The issues do reference packages
  3. So, the assignment of the package identifiers start when issues are serialized, and the ordering of
    the issues determines the IDs of the referenced packages.
  4. As this commit adds a new first issue, the package referenced gets the first id: 0.

So, it seemed natural to just change property order so that packages are serialized before the issues, but
that would re-create a similar issues, because there is a cyclic reference between these instances.

@sschuberth what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable as an explanation, good enough for me, thanks!

@fviernau fviernau requested a review from sschuberth April 24, 2024 09:52
@fviernau fviernau changed the title test(reporters): Add two scan issues to report test assets test(reporters): Add three scan issues to report test assets Apr 24, 2024
@fviernau fviernau merged commit c091056 into main Apr 25, 2024
22 checks passed
@fviernau fviernau deleted the webapp-filter-excluded-preparations branch April 25, 2024 07:11
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.

2 participants