Use `isEmpty()` and simplified assertions in Test #8473

StefanSpieker · 2023-09-10T14:13:09Z

Use isEmpty() and simplified assertions in Test

Testing done

Unit Tests still pass locally

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
There is automated testing or an explanation as to why this change has no tests.
New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

Before the changes are marked as ready-for-merge:

Maintainer checklist

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

MarkEWaite · 2023-09-11T03:17:52Z

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

jtnord · 2023-09-11T12:10:27Z

test/src/test/java/hudson/PluginManagerTest.java

@@ -487,7 +487,7 @@ private String callDependerValue() throws Exception {
        // Check that the basic API endpoint invocation works.
        assertEquals("ok", response.getString("status"));
        JSONArray data = response.getJSONArray("data");
-        assertTrue(data.size() > 0);
+        assertFalse(data.isEmpty());


both of these are not great - if it fails it does not give you hints as to why it fails - what was in the data is a great hint for analysis.

E.g.
assertThat(data, empty()) would be preferable here as it would produce something like expected an empty collection but got ["some", "Collection", "with", "values"] (hamcrest javadoc, other asserts libraries also available)

y'all should have bashed me over the head.
as this would only fail if the collection was empty there is no additional information to be gleaned in this case. An empty collection is an empty collection no matter what diagnostics you add.
The inverse (when checking it is empty) is not the case - but these where asserting something was not empty, not that it was empty.

My apologies @StefanSpieker

I guess we all got a little confused :) But now it is really better than before :)

NotMyFault · 2023-09-16T18:52:34Z

@StefanSpieker Would you mind addressing the feedback from jtnord?

…ase of failure

jtnord · 2023-09-18T14:35:49Z

something is not empty (and we have some info about it)!

https://github.com/jenkinsci/jenkins/pull/8473/checks?check_run_id=16857757226

test/src/test/java/hudson/PluginManagerTest.java

test/src/test/java/hudson/model/HelpLinkTest.java

test/src/test/java/lib/hudson/ListScmBrowsersTest.java

Co-authored-by: James Nord <[email protected]>

jtnord · 2023-09-25T15:10:19Z

/label ready-for-merge

comment-ops-bot · 2023-09-25T15:10:22Z

I wasn't able to add the following labels: ready

Check that the label exists and is spelt right then try again.

jtnord · 2023-09-25T15:10:55Z

/label ready-for-merge

use isEmpty() and simplified assertions

1963995

NotMyFault added the skip-changelog Should not be shown in the changelog label Sep 10, 2023

MarkEWaite approved these changes Sep 10, 2023

View reviewed changes

MarkEWaite self-assigned this Sep 10, 2023

NotMyFault approved these changes Sep 10, 2023

View reviewed changes

MarkEWaite added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 11, 2023

jtnord reviewed Sep 11, 2023

View reviewed changes

NotMyFault removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 12, 2023

StefanSpieker added 2 commits September 16, 2023 21:41

use assertThat instead of assertFalse to get better messages in c…

8be11e5

…ase of failure

Merge branch 'master' into minor_refactoring_in_tests

dade9df

jtnord reviewed Sep 18, 2023

View reviewed changes

StefanSpieker and others added 10 commits September 24, 2023 10:12

Update test/src/test/java/lib/hudson/ListScmBrowsersTest.java

70bcd90

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/hudson/PluginManagerTest.java

b1c88de

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/hudson/PluginManagerTest.java

b873d4e

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/hudson/PluginManagerTest.java

8b41c39

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/hudson/model/HelpLinkTest.java

8659ff6

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/hudson/model/HelpLinkTest.java

716705b

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/lib/hudson/ListScmBrowsersTest.java

a227b71

Co-authored-by: James Nord <[email protected]>

Update test/src/test/java/lib/hudson/ListScmBrowsersTest.java

2cdf9d2

Co-authored-by: James Nord <[email protected]>

Merge branch 'master' into minor_refactoring_in_tests

c9ccf14

spotless fix

fc1a29d

jtnord approved these changes Sep 25, 2023

View reviewed changes

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 25, 2023

NotMyFault merged commit 87888c7 into jenkinsci:master Sep 27, 2023
16 checks passed

StefanSpieker deleted the minor_refactoring_in_tests branch September 27, 2023 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `isEmpty()` and simplified assertions in Test #8473

Use `isEmpty()` and simplified assertions in Test #8473

StefanSpieker commented Sep 10, 2023 •

edited by gitpod-io bot

Loading

MarkEWaite commented Sep 11, 2023

jtnord Sep 11, 2023 •

edited

Loading

jtnord Sep 18, 2023 •

edited

Loading

StefanSpieker Sep 24, 2023

NotMyFault commented Sep 16, 2023

jtnord commented Sep 18, 2023

jtnord commented Sep 25, 2023 •

edited

Loading

comment-ops-bot bot commented Sep 25, 2023

jtnord commented Sep 25, 2023

Use isEmpty() and simplified assertions in Test #8473

Use isEmpty() and simplified assertions in Test #8473

Conversation

StefanSpieker commented Sep 10, 2023 • edited by gitpod-io bot Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

MarkEWaite commented Sep 11, 2023

jtnord Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

jtnord Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

StefanSpieker Sep 24, 2023

Choose a reason for hiding this comment

NotMyFault commented Sep 16, 2023

jtnord commented Sep 18, 2023

jtnord commented Sep 25, 2023 • edited Loading

comment-ops-bot bot commented Sep 25, 2023

jtnord commented Sep 25, 2023

Use `isEmpty()` and simplified assertions in Test #8473

Use `isEmpty()` and simplified assertions in Test #8473

StefanSpieker commented Sep 10, 2023 •

edited by gitpod-io bot

Loading

jtnord Sep 11, 2023 •

edited

Loading

jtnord Sep 18, 2023 •

edited

Loading

jtnord commented Sep 25, 2023 •

edited

Loading