Skip to content

Conversation

crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Dec 3, 2024

Note

This PR is built on top of #415

This PR updates plugin directory tests to call api.wordpress.org directly instead of using cached content.

Since there are 44k+ plugins on wordpress.org and the whole tests take 90 minutes, the full tests run on nightly build by default. In regular CI builds and local development, the tests only verify a small amount (20) of plugins. You can set TEST_ALL_PLUGINS env to run full tests locally.

A whole bunch of tests that verifies weird JSON response values are removed in this PR, but they will be added back later in a separate PR. Update: I have added them in this PR.

@crazytonyli crazytonyli marked this pull request as ready for review December 4, 2024 01:11
Base automatically changed from wordpress-org-plugin-directory to trunk December 4, 2024 12:15

let mut query_plugins_failures = Vec::new();
let mut all_slugs: Vec<String> = Vec::new();
for page in 1..=total_pages {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we shouldn't hit the endpoint at all if we're not running a full test.

I see you have a json file for testing, can we have one of those for this so that it runs quickly? We're just interested in testing the parsing code, so I'd love to factor out networking for the vast majority of cases if we can

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO we shouldn't hit the endpoint at all if we're not running a full test.

Considering this test is part of integration tests, I think it's okay for it to make a small amount of HTTP requests? There is still value in running this test during PR (and locally development), like running the entire test function to ensure there is not obvious issue in the test code itself.

I see you have a json file for testing, can we have one of those for this so that it runs quickly?
I have added one in 022fa1f as part of unit tests.

@crazytonyli crazytonyli force-pushed the wordpress-org-plugin-directory-serial-tests branch from 27b40f4 to 7a443c0 Compare December 4, 2024 20:11
@crazytonyli crazytonyli requested a review from jkmassel December 4, 2024 23:45
@oguzkocer
Copy link
Contributor

@crazytonyli I'll try to review this one tomorrow. Sorry for the delay!

Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

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

I left a few minor suggestions, one of which is enabling a test that was missing the #[test] attribute. As it turns out, that test is currently failing.

I also tested the integration tests - without the TEST_ALL_PLUGINS environment variable - and even though the test reports that it was successful, there were actually parsing errors:

Only a small amount of plugins will be fetched.
F(https://api.wordpress.org/plugins/info/1.2/?action=query_plugins&request[per_page]=10&request[page]=1)F(https://api.wordpress.org/plugins/info/1.2/?action=query_plugins&request[per_page]=10&request[page]=2)
Fetching and parsing 0 plugins...

2 query plugins failures:

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.59s

We should report the test as failure in such a case.


I really don't like using environment variables for configuration. In general, I feel like environment variables are very convenient, but they can also cause a headache. There are many places it can be set from and they are hard to track. Furthermore, the behavior change in the code can feel magical, because it's not clear why it changed unless you know the existence of an environment variable - meaning it lacks intentionality.

I've been very deliberate in avoiding environment variables until now and I'd strongly prefer us continue to do so. We can merge this PR with the environment variable, but I'd like to try and open a follow up PR to replace it. Let me know if you have any concerns about that plan.

@crazytonyli
Copy link
Contributor Author

@oguzkocer Good catch. I have fixed all the issues.

Let me know if you have any concerns about that plan.

Not at all.

@crazytonyli crazytonyli merged commit 037fba2 into trunk Dec 11, 2024
23 checks passed
@crazytonyli crazytonyli deleted the wordpress-org-plugin-directory-serial-tests branch December 11, 2024 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants