Similar to #2930442: Base test classess should extend BrowserTestBase

PHPUnit initiative is moving away from WebTestBase and towards Functional tests based on BrowserTestBase, then deprecating the old base classes, we should probably follow their lead, although I dont think we have to keep the old ones since we are in alpha stil.

CommentFileSizeAuthor
#18 ga_login-phpunit.png46.92 KBdaggerhart
#15 2938091-12-15-interdiff.txt657 bytesdaggerhart
#15 2938091-15.patch42.36 KBdaggerhart
#12 2938091-7-12-interdiff.txt405 bytesdaggerhart
#12 2938091-12.patch42.12 KBdaggerhart
#9 2938091-9.patch42.42 KBManuel Garcia
#9 interdiff-2938091-7-9.txt713 bytesManuel Garcia
#7 2938091-7.patch41.72 KBdaggerhart
#2 2938091-2.patch3.92 KBdaggerhart
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daggerhart created an issue. See original summary.

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
Status: Active » Needs work
FileSize
2938091-2.patch3.92 KB

Very much a work in progress patch. I ran into trouble around the config schema again and ran out of time.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented

This should fix the schema errors @daggerhart: #2948999: Incorrect schema definition

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review

Triggering test bot

Status: Needs review » Needs work

The last submitted patch, 2: 2938091-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
Assigned: Unassigned » daggerhart

Thanks @manuel-garcia, that fixes my problem!

I'm going to assign this issue to me because I have a bit of work into it and want to complete my first pass. Should have something to show by eod.

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
Assigned: daggerhart » Unassigned
Status: Needs work » Needs review
FileSize
2938091-7.patch41.72 KB

Here are 4 working tests classes. I've removed the old test classes as well.

There were some minor changes I had to make to the setup and validation plugins in order to get these tests working how I wanted. Mainly changing some protected properties/methods to public.

I have not included a test for Recovery Codes, because they are technically a TFA validation plugin.

Feedback welcome. Unassigning myself incase someone else wants to take a crack at this.

Status: Needs review » Needs work

The last submitted patch, 7: 2938091-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2938091-7-9.txt713 bytes
2938091-9.patch42.42 KB

Let's see if this gets the tests running on drupal.org

Status: Needs review » Needs work

The last submitted patch, 9: 2938091-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented

Moving this to alpha5, when tfa will have a release that contains the new TfaTestBase class.

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
Status: Needs work » Needs review
FileSize
2938091-12.patch42.12 KB
2938091-7-12-interdiff.txt405 bytes

Since we figured out the issue with d.o testing, I like the idea of removing the test_dependencies: from info file. We are currently misusing test_dependencies in the module.

Here is a re-roll of #7 with the info file changed to match @manuel-garcia's #9. I re-rolled against #7 because for some reason #9 failed to apply because of composer.json changes that have happened in the 8.x-1.x branch.

The only remaining question I have is: do we need to add drupal/tfa as a requirement in composer.json as Manuel did in #9? I honestly don't know, so any feedback would be appreciated.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented

I'd say having a composer dependency on tfa makes sense because:

  • Although you could enable the module without tfa and get no errors, the module does nothing without tfa enabled.
  • With the composer dependency people could then just require drupal/ga_login and conviently get all its dependencies.
Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented 
+++ b/ga_login.info.yml
@@ -4,9 +4,4 @@ package: System
- - tfa (>=8.x-1.0-alpha3)

I would also have a dependency on tfa on the info file for the same reasons.

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
FileSize
2938091-15.patch42.36 KB
2938091-12-15-interdiff.txt657 bytes

Thanks Manuel, that all makes good sense to me.

Here is a re-roll that adds tfa as a composer dependency, and re-adds the tfa-sepcific module version into the info file.

Since old versions of tfa will not work with new versions of ga_login, I think it's best that we bear the burden of ensuring that users have the right versions of the modules that work together. This means we'll have to continue to bump the required tfa version in the info file for each alpha release, but I think it's worthwhile to save module users from the frustration of figuring it out on their own.

Manuel Garcia’s picture

Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Makes sense to me @daggerhart - I've got no more things to complain about so RTBCing :)

nerdstein’s picture

nerdstein
he/him
Primary language English
Location United States
CreditAttribution: nerdstein as a volunteer and at Hook 42 commented

This patch looks great - thanks. Please verify that things are registering as expected in the CI bot and this is good to go!

daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
FileSize
ga_login-phpunit.png46.92 KB

Verified the tests are being picked up, running and passing. Merging now, thanks!

  • daggerhart committed 0c860fe on 8.x-1.x 
    Issue #2938091 by daggerhart, Manuel Garcia: Convert tests to use...
daggerhart’s picture

daggerhart CreditAttribution: daggerhart commented
Status: Reviewed & tested by the community » Fixed

Merged into 8.x-1.x

Status: Fixed » Closed (fixed)

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