Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase TOTP coverage #469

Merged
merged 3 commits into from
Oct 12, 2022
Merged

Increase TOTP coverage #469

merged 3 commits into from
Oct 12, 2022

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Oct 12, 2022

This adds some tests to fill small gaps in coverage, and marks some functions as ignored because they're not testable in a unit test, or aren't worth testing. This increases the coverage of class-two-factor-totp.php to 92%, making it obvious that it is well-tested.

To aid in writing tests, this also adds phpunit-watcher, similar to Gutenberg, WordCamp.org, and some other WordPress repos.

See #468

@iandunn
Copy link
Member Author

iandunn commented Oct 12, 2022

I'll take a closer look tomorrow at the lint-php8 failure tomorrow.

@kasparsd
Copy link
Collaborator

The PHP8 linter issues are fixed in #466.

composer.json Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
phpunit-watcher.yml.dist Outdated Show resolved Hide resolved
@jeffpaul jeffpaul linked an issue Oct 12, 2022 that may be closed by this pull request
These functions are either untestable in a pure unit test, or aren't worth testing. They're better suited to manual testing and e2e tests.
@iandunn
Copy link
Member Author

iandunn commented Oct 12, 2022

I removed the commits related to phpunit-watcher and rebased against master, so I think this is ready for review again. I'll open a new PR w/ the other commits and do some of the other PHP tooling work there.

Copy link
Collaborator

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

Looks great!

@iandunn
Copy link
Member Author

iandunn commented Oct 12, 2022

Great, should I go ahead and merge it then? Or is there anything else we need to do first?

@kasparsd
Copy link
Collaborator

Yes, please merge it in!

@iandunn iandunn merged commit 7757411 into master Oct 12, 2022
@iandunn iandunn deleted the add-totp-tests branch October 12, 2022 19:08
@jeffpaul jeffpaul added this to the 0.8.0 milestone Oct 12, 2022
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.

Test coverage seems low
3 participants