Problem/Motivation

I have noticed that in all of the tests for photoswipe whenever there is a call to the method $session->waitForElementVisible() it is missing the appropriate $this->assertNotNull() assertion around it. If this assertion is not made, the $session->waitForElementVisible() method will not throw an error and continue even if the element is not visible after the specified timeout. This might have implications like tests turning out OK when in reality they are not.

Steps to reproduce

Proposed resolution

Wrap all calls to this method in the $this->assertNotNull() assertion.
Example: $this->assertNotNull($session->waitForElementVisible('css', '.some-class'));

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork photoswipe-3387336

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

LRWebks created an issue. See original summary.

Anybody made their first commit to this issue’s fork.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: grevil » lrwebks
Priority: Minor » Normal

@LRWebs please prepare the MR for @Grevil to review. I'll give it a first shot as an example to safe us from misunderstandings.

Anybody opened merge request !94

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Active » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Assigned: lrwebks » grevil
Category: Task » Bug report
Status: Needs review » Needs work

Funny, two test indeed fail and I think we don't expect the returned element to be NULL, perhaps we found two testing issues. Assigning this to Grevil for detailed checks.

Thanks @LRWebks, nothing left to do so far :)

Changing to a possible bug

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@Grevil: I think we also have such tests in other projects, especially COOKiES? If yes, please also open such an issue there.
Important finding :)

grevil’s picture

grevil commented

Nice find! I'll take a look, once I fixed up the GitLab CI test issue: #3390116: Make Gitlab CI Pipeline succeed.

grevil’s picture

grevil commented

Ok, let's see what the new CI pipeline has to say :)

grevil’s picture

grevil commented
Status: Needs work » Reviewed & tested by the community

All green again! Just a few incorrect assertions, nothing to worry about. LGTM!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Confirming RTBC!

  • Grevil committed 96a0c39a on 5.x authored by Anybody 
    Issue #3387336: Fix assertion issue in automated tests
grevil’s picture

grevil commented
Status: Reviewed & tested by the community » Fixed
grevil’s picture

grevil commented
Assigned: grevil » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.