Problem/Motivation

To refresh the update status data, the base class visits the update status page, clicks a link, waits for meta refresh, which then redirects back to the update status page.

We can refresh the data using the API and request the page once it's done.

This test is also probably a candidate for converting to a kernel test, although there's a handful of base classes and traits that all need to be shifted at the same time to do that. This is a small step in that direction because it removes a batch page.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3615571

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

catch created an issue. See original summary.

catch opened merge request !16615

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

That looks like a relevant failure unfortunately.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Needs the $ui_only parameter for that method too.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Wait apparently that's not enough...

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

quietone’s picture

quietone commented
Status: Needs work » Needs review

Just needed to restore a drupalGet to the ui_only part.

catch’s picture

catch
he/him
Primary language English
commented

For $ui_only we're often on the page before we call the method, so it might be better to add the drupalGet() before calling the method in just the place that needs it.

catch’s picture

catch
he/him
Primary language English
commented

#8 is right and #9 is wrong, $ui_only visits the check page, so we always need to go back to admin/reports/updates even if we were there before calling the method.

Pushed one commit for UpdateContribTest which appears to need to go via the UI, maybe because it's a newly enabled module. MR is back to green again now.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I found a lingering drupalGet() that I suspect was made redundant by the #8 changes.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Dropped that one.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

That's the only thing I found. Thank you.

mstrelan’s picture

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

I think by fetching via the API without separate drupalGet calls we run in to issues with service properties not being reset in tests that call refreshUpdateStatus more than once. In this case, UpdateProcessor::createFetchTask stores project names in $this->fetchTasks, which is not cleared between calls to refreshUpdateStatus. So from the second call on, nothing is queued and fetchData() does nothing. The tests pass because drupalGet('admin/reports/updates') calls getAvailable(TRUE), which fetches any missing data. I found this with the help of an LLM and verified by stepping through with Xdebug.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Pushed a fix that resets the properties, and verified that subsequent calls now fetch via the API rather than the drupalGet call. It doesn't make the test any faster, but the API path now actually does the fetching.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Not sure how we could 'test the test' here, but better to fix it than not. Maybe some of the non-ui/batch things can move to a kernel test later on. I think I can re-RTBC the most recent change even though the rest of this was mostly mine.