Skip to content

test: lint and fix wpunit tests#2455

Merged
jasonbahl merged 11 commits intowp-graphql:developfrom
justlevine:tests/backported-fixes-and-lints
Jul 25, 2022
Merged

test: lint and fix wpunit tests#2455
jasonbahl merged 11 commits intowp-graphql:developfrom
justlevine:tests/backported-fixes-and-lints

Conversation

@justlevine
Copy link
Copy Markdown
Collaborator

@justlevine justlevine commented Jul 24, 2022

What does this implement/fix? Explain your changes.

This PR lints and applies some basic fixes/refactors to existing WPUnit tests.
Work on #2408 exposed many cases of data being leaked between tests, however fixing the issues there makes PR review nearly unusable due to both the scope, and unrelated diffs from linting. It also makes merging/rebasing a PITA.

There are also numerous cases where tests are passing even though they are written incorrectly or there is a bug in the plugin, due to leaky data.

#2408 is not the first PR where I've encountered this, and I believe this will make future work significantly more efficient. While this may cause some additional merge conflicts on already-open PRs, many of those already have merge conflicts that must be dealt with anyway.

To test:

Add the following method to the bottom of WPHelperTest.php (so it runs last), and run vendor/bin/codecept run wpunit -vvv.

public function testEnsureTypeCleanup() {
  $post_types = get_post_types();
  codecept_debug( $post_types );
  foreach ( $post_types as $post ) {
    codecept_debug( $post );
    codecept_debug( get_posts( [ 'post_type' => $post ] ) );
  }

  $taxonomies = get_taxonomies();
  codecept_debug( $taxonomies );
  foreach ( $taxonomies as $taxonomy ) {
    codecept_debug( $taxonomy );
    codecept_debug( get_terms( [ 'taxonomy' => $taxonomy ] ) );
  }

  codecept_debug( get_users() );
}

Only default WP post types and taxonomies should appear, the admin user, and no posts or terms.

Does this close any currently open issues?

Any relevant logs, error output, GraphiQL screenshots, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?

  • While this touches many wpunit tests, it does not touch all of them, only those affected by bugs/leaky types discovered in fix: Connection pagination refactor #2408. This is intentional, in an attempt to limit as many merge conflicts as possible.

Where has this been tested?

Operating System: Ubuntu 20.04 (wsl2 + devilbox + PHP 8.0.19)

WordPress Version: 6.0.1

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5402 lines exceeds the maximum allowed for the inline comments feature.

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 24, 2022

Coverage Status

Coverage remained the same at 81.583% when pulling 41fc44e on justlevine:tests/backported-fixes-and-lints into 18838ea on wp-graphql:develop.

@justlevine justlevine changed the title tests: lint and fix wpunit tests test: lint and fix wpunit tests Jul 24, 2022
Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5730 lines exceeds the maximum allowed for the inline comments feature.

@justlevine justlevine force-pushed the tests/backported-fixes-and-lints branch from d6d11dd to b2215d5 Compare July 24, 2022 13:06
Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5749 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5901 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5882 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 5948 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 8645 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 9342 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 9356 lines exceeds the maximum allowed for the inline comments feature.

@justlevine justlevine force-pushed the tests/backported-fixes-and-lints branch from 9e3640c to a53561c Compare July 24, 2022 19:56
@justlevine
Copy link
Copy Markdown
Collaborator Author

@jasonbahl

I'm almost at the point in this PR where additional fixes to the tests (leaked info, broken logic, etc) will expose existing bugs in the plugin and cause CI tests to fail until they're addressed in the code.
Should I include those fixes (to the tests, not the bugs) in this PR, or save them for separate PRs that address those bugs?

@justlevine justlevine self-assigned this Jul 24, 2022
@justlevine justlevine added scope: tests Developing unit tests, integration tests, and ensuring coverage needs: reviewer response This needs the attention of a codeowner or maintainer labels Jul 24, 2022
Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 9432 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Copy Markdown

@qlty-cloud-legacy qlty-cloud-legacy bot left a comment

Choose a reason for hiding this comment

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

The PR diff size of 9453 lines exceeds the maximum allowed for the inline comments feature.

@qlty-cloud-legacy
Copy link
Copy Markdown

Code Climate has analyzed commit 41fc44e and detected 0 issues on this pull request.

View more on Code Climate.

@justlevine justlevine marked this pull request as ready for review July 25, 2022 00:49
@justlevine justlevine requested a review from jasonbahl July 25, 2022 00:50
@justlevine justlevine added the status: in review Awaiting review before merging or closing label Jul 25, 2022
@jasonbahl jasonbahl merged commit cd7819d into wp-graphql:develop Jul 25, 2022
@justlevine justlevine deleted the tests/backported-fixes-and-lints branch September 24, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: reviewer response This needs the attention of a codeowner or maintainer scope: tests Developing unit tests, integration tests, and ensuring coverage status: in review Awaiting review before merging or closing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants