Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make check of logged-in user independent of UI #631

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

fcojfernandez
Copy link
Member

Making the check of the logged user using Jenkins API instead the UI, so the check is more robust in case something is not well rendered.

@bmunozm @EstherAF

@jtnord I think you're the new maintainer?

@jglick jglick changed the title Make check of logged user independent from UI Make check of logged-in user independent of UI Jan 19, 2021
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

IIUC uses

super(context, context.url("me/"));
}
public User(Jenkins context, String name) {
super(context, context.url("user/%s/", name));
}
private void load() {
if (id != null) return;
try {
JsonNode json = getJson();
id = json.get("id").asText();

@fcojfernandez
Copy link
Member Author

When the user is not valid, the User object is not null but its id is. User seems to be null only if there has been no attempt to login. That made LdapPluginTest failed in a couple of tests.

Already Fixed

@@ -201,7 +201,8 @@ public boolean matchesSafely(PageObject item) {
@Override
public boolean matchesSafely(final Jenkins jenkins) {
final User currentUser = jenkins.getCurrentUser();
return currentUser != null && currentUser.id().equals(user);
// if the user is not logged, currentUser can be not null with a null id
return currentUser != null && currentUser.id() != null && currentUser.id().equals(user);
Copy link
Member

Choose a reason for hiding this comment

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

or just

Suggested change
return currentUser != null && currentUser.id() != null && currentUser.id().equals(user);
return currentUser != null && user.equals(currentUser.id());

I suppose

Copy link
Member Author

Choose a reason for hiding this comment

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

that was my first intention, but after a search I couldn't be 100% sure that user will be always non-null. I can commit the suggestion if you prefer

@amuniz
Copy link
Member

amuniz commented Jan 20, 2021

There are currently 264 test failures in master, this PR has 330. Is the diff because of this change?

@fcojfernandez
Copy link
Member Author

fcojfernandez commented Jan 20, 2021

After analysing the failures (and I'm not experienced on the plugins whose tests are failing and I might be wrong):

  • None of those tests are checking the logged user, that is what is changed in this PR
  • Only AuditTrailPluginTest and MissionControlTest are failing in local. The rest of them are passing using the latest LTS in a local execution. Why are the tests failing then? I don't know and I don't have knowledge about those plugins to know if there's something in the test per se (my guess)

The details:

  1. PlotPluginPropertiesTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is OK
  2. JobDslPluginTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is OK
  3. PrioritySorterPluginTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is OK
  4. MissionControlTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is failing as well, but the tests are executed as anonymous user. NOT RELATED
  5. FtpPublishPluginTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is OK
  6. PlotPluginXmlTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is OK
  7. AuditTrailPluginTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is is failing as well, but the tests are executed as anonymous user. NOT RELATED
  8. core.BuildHistoryTest: Not using either Login.doSuccessfullLogin or Matchers.hasLoggedInUser / Matchers.loggedInAs. Local execution with latest LTS is OK

@timja
Copy link
Member

timja commented Jan 20, 2021

Plot plugin is known to be broken on weeklies. It should probably be removed from ATH, I don't think it meets the threshold for what should be here.

@amuniz
Copy link
Member

amuniz commented Jan 20, 2021

Great. Thanks @fcojfernandez! I'm merging now.

@amuniz amuniz merged commit ce2ff85 into jenkinsci:master Jan 20, 2021
@fcojfernandez fcojfernandez deleted the logged-user-no-ui branch January 20, 2021 12:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants