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

Add Pipeline Graph View to the managed set #3137

Merged

Conversation

alecharp
Copy link
Member

@alecharp alecharp commented Apr 25, 2024

NOTE: Not adding weekly test label here now because of known failures.

This pull request adds Pipeline Graph View plugin on the managed set.

Fixes #3136

Testing done

I ran

PLUGINS=pipeline-graph-view bash local-test.sh

It has 2 tests in error in the 2.455 (weekly) line, which I can reproduce on the plugin directly (not related to PCT).
Working on those now.

Submitter checklist

@alecharp
Copy link
Member Author

The two tests failing are:

  • PipelineStepApiLegacyTest#getAllStepsReturnsStepsForComplexParallelBranches
  • PipelineStepApiTest#getAllStepsReturnsStepsForComplexParallelBranches

Both are now failing because of an additional step when the pipeline defined by https://github.com/jenkinsci/pipeline-graph-view-plugin/blob/main/src/test/resources/io/jenkins/plugins/pipelinegraphview/utils/complexParallelSmokes.jenkinsfile. The additional step is making mention of Get contextual object from internal APIs.

@alecharp
Copy link
Member Author

alecharp commented Apr 25, 2024

The tests are failing as soon as the fix from jenkinsci/pipeline-model-definition-plugin#700 is integrated.

How to reproduce:

  • fix jenkins.version from 2.440 to 2.440.1
  • bump bom version to 2961.v1f472390972e (at this moment the tests are passing)
  • bump pipeline-model-definition, pipeline-model-api, pipeline-model-extensions and pipeline-stage-tags-metadata to 2.2198.v41dd8ef6dd56

@basil
Copy link
Member

basil commented Apr 25, 2024

Since the fix in jenkinsci/pipeline-graph-view-plugin#392 will apply only to 2.440.x, I recommend testing this PR with full-test rather than weekly-test.

@alecharp
Copy link
Member Author

With latest commit, the tests are not failing locally anymore. Testing with CI.

@alecharp alecharp added full-test Test all LTS lines in this PR and do not halt upon first error. and removed full-test Test all LTS lines in this PR and do not halt upon first error. labels Apr 26, 2024
@alecharp
Copy link
Member Author

5 tests in display-url-api plugin are not passing due to the addition of the pipeline-graph-view plugin here. I can reproduce those failures locally in BOM so I'm looking to find what is wrong.

@alecharp
Copy link
Member Author

alecharp commented Apr 26, 2024

When the pipeline-graphp-view plugin is installed, the test implementations of the DisplayURLProvider are not used when calling DisplayURLProvider.get(), but available in the DisplayURLProvider.all()

@alecharp
Copy link
Member Author

Here is the testing I've been doing to resolve the problem:

  • updating parent pom, jenkins.version and bom on display-url-api plugin
  • adding pipeline-graph-view plugin as a test dependency to display-url-api plugin (creates a cycling dependency but for test purpose it works
  • running the test class ActionRedirectExtendedTest in display-url-api plugin
  • in debug mode, breaking in the first statement of the first test, we can see:
    • DisplayURLProvider#all() returns a list of 4 implementations
    • the code path in DisplayURLDecorator#decorate is the same for both configuration (with and without the plugin)
    • when logging the headers in the tests (adding .log().headers()), we can see that both are resolved using the AbstractDisplayAction#doRedirect

I think the problem is that in DisplayURLProvider#getPreferredProvider, we are streaming in all the implementation of DisplayURLProvider. The implementation in pipeline-graph-view plugin is the first in the list, because of the ordinal and https://github.com/jenkinsci/display-url-api-plugin/blob/d3e2e182cdfb9919b122813c827be9d6e929685e/src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java#L221-L223 is returning that implementation when the test in the plugin expect https://github.com/jenkinsci/display-url-api-plugin/blob/d3e2e182cdfb9919b122813c827be9d6e929685e/src/test/java/org/jenkinsci/plugins/displayurlapi/actions/ActionRedirectExtendedTest.java#L101 to be the first implementation.

The TestExtension has no was to get an ordinal. We could change the test to configure the preferred implementation to make sure it's using the test extension.

@alecharp
Copy link
Member Author

alecharp commented Apr 29, 2024

I might have a patch for the display-url-api plugin but I need to confirmed that it's not reducing the code coverage.

EDIT: the patch basically sets a global preferred URL provider, and so bypass the automatic selection of the first implementation available.

@alecharp
Copy link
Member Author

I opened jenkinsci/display-url-api-plugin#212 for review.

@alecharp
Copy link
Member Author

I validated the content of jenkinsci/display-url-api-plugin#212 by adding --local-checkout-dir for display-url-api in pct.sh and running local-test.sh script with PLUGINS set to display-url-api locally. All test passed.

@alecharp
Copy link
Member Author

Once #3144 is merged, we can update this pull request and add the full-test label again to validate it.

@basil basil added the full-test Test all LTS lines in this PR and do not halt upon first error. label Apr 29, 2024
@basil basil marked this pull request as ready for review April 29, 2024 19:19
@basil basil requested a review from a team as a code owner April 29, 2024 19:19
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@basil basil enabled auto-merge (squash) April 29, 2024 19:19
@basil basil changed the title Adds Pipeline Graph View plugin to the managed set Add Pipeline Graph View to the managed set Apr 29, 2024
@alecharp
Copy link
Member Author

I'm glad I could help.

@basil basil merged commit e92efb2 into jenkinsci:master Apr 29, 2024
712 of 713 checks passed
@basil basil added the enhancement New feature or request label Apr 29, 2024
@basil
Copy link
Member

basil commented Apr 30, 2024

Still failing on the main branch pending jenkinsci/pipeline-graph-view-plugin#394.

@basil
Copy link
Member

basil commented Apr 30, 2024

If perchance anyone happens to be awake in the next few hours and interested in helping, it would be great to take advantage of time zones to merge and release the above and restart a build of https://ci.jenkins.io/job/Tools/job/bom/job/master/ which will take several hours.

@basil
Copy link
Member

basil commented Apr 30, 2024

Specifically the test was relying on the branch being named something other than master, which was true for https://github.com/jenkinsci/pipeline-graph-view-plugin where the branch is named main and was also true for #3137 where the branch was named PR-something-or-other, but not true for https://github.com/jenkinsci/bom where the branch is actually named master

@alecharp alecharp deleted the feature/add-pipeline-graph-view-plugin branch April 30, 2024 09:27
@MarkEWaite
Copy link
Contributor

I've started the release build on ci.jenkins.io and set the branch protection to lock the master branch. Will watch for the completion of the release.

@alecharp
Copy link
Member Author

alecharp commented Apr 30, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request full-test Test all LTS lines in this PR and do not halt upon first error.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Pipeline Graph View to the managed set
3 participants