Skip to content
This repository has been archived by the owner on Jan 4, 2023. It is now read-only.

[FIX JENKINS-39842] Open Blue Ocean button should not try to load /activity for a folder #96

Merged
merged 2 commits into from Jan 6, 2017

Conversation

tfennelly
Copy link
Member

Description

Test for jenkinsci/blueocean-plugin#685

Clicking on the "Open Blue Ocean" button while in a classic folder that's not a MBP project folder should bring the user to the main top-level blue ocean page i.e. pipelines

See JENKINS-39842.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
  • Ran Acceptance Test Harness against PR changes.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

Clicking on the "Open Blue Ocean" button while in a classic folder that's not a MBP project folder should bring the user to the main top-level blue ocean page i.e. pipelines

// Go to a folder along the path to the MBP, but one
// of the parent folders i.e. not the MBP project folder.
classicGeneral.navigateToRun('job/anotherFolder/job/三百/job/ñba');
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not to job/anotherFolder ? Maybe also test that?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but not needed imo i.e. the main thing is that it's not the MBP folder at the end of the path.

@kzantow
Copy link
Collaborator

kzantow commented Jan 6, 2017

Confusing folder for the layperson, maybe just simplify?

@tfennelly
Copy link
Member Author

Okay ... let me see if I can create a separate test completely that just tests this. Seemed a bit overkill when I was doing it (piggybacking was easier), but what the heck :)

@kzantow
Copy link
Collaborator

kzantow commented Jan 6, 2017

I mean... why would you include random UTF characters for a folder? it's confusing to tell what it even represents. Just use a basic folder for this test, wouldn't that make the most sense?

@tfennelly
Copy link
Member Author

@kzantow that folder structure was already there and, I assume, the weird characters were added so as to test url encodings when strange characters are in use i.e. it's a good idea having them in the test.

@tfennelly
Copy link
Member Author

Tbh, I found the strange chars to be more interesting than confusing, when I first saw it there. What do they mean ... no idea ... doesn't matter ... they're just characters in a file path... who cares?? I seriously doubt you find it all that confusing either hehehe 😉

Copy link
Collaborator

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Seems fine (I would still like to see an obvious name for the non-MBP folder)

@tfennelly
Copy link
Member Author

@kzantow I'm creating another test here that has a folder path structure using chars ... hang fire ... c&p in action 😉

Using folder and job/branch names that are not confusing (special chars etc). BTW I'm leaving the original tests that were using weird chars because I think it's good to have tests that challenge the url encoding etc. Sure that can result in the presence of weird chars, but weird chars are a fact of life, so good to test against them.
@tfennelly
Copy link
Member Author

@kzantow another test added that's using more "normal" characters. However I'm keeping the original tests because imo it's good to have tests using "weird" chars.

Copy link
Collaborator

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

you are the man, @tfennelly

@tfennelly
Copy link
Member Author

@kzantow thanks for the review bud!!

@tfennelly tfennelly merged commit 1ea16a7 into master Jan 6, 2017
@tfennelly tfennelly deleted the JENKINS-39842-open-bo-folder branch January 6, 2017 16:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants