Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In #2925064: [1/2] JS codestyle: no-restricted-syntax we broke Drupal.behaviors.copyFieldValue(). It was fixed in #2941106: Site email address in the install profile form is no longer copied to the user email address but we should add test coverage so we don't break it again.
Proposed resolution
Add test coverage.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#59 | interdiff.txt | 1.57 KB | lauriii |
#59 | 2944089-59.patch | 4.1 KB | lauriii |
| |||
#57 | interdiff_56-57.txt | 2.29 KB | Tanuj. |
#57 | 2944089-57.patch | 4.2 KB | Tanuj. |
| |||
#56 | interdiff_51_56.txt | 2.35 KB | Abhisheksingh27 |
Comments
Comment #2
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #3
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedI tried writing Javascript test case for
Drupal.behaviours.copyFieldValue
Here is in progress code in gist https://gist.github.com/msankhala/e6bed178e64c4369b755eb4ef7a76088I took
core/tests/Drupal/FunctionalJavascriptTests/Core/Installer/Form/SelectProfileFormTest.php
test case as starting point for this. I simply copiedcore/tests/Drupal/FunctionalJavascriptTests/Core/Installer/Form/SelectProfileFormTest.php
to
core/tests/Drupal/FunctionalJavascriptTests/Core/Installer/Form/ConfigureSiteFormTest.php
and created new method calledtestSiteEmailCopiedToAccountEmail()
and started writing test scenario in this method.I am not sure how to wait till Drupal installation is complete and redirected to Configure Site screen, where actually
Drupal.behaviours.copyFieldValue
is in action. Can someone guide me in this?Or this should go into System module. Should i create new module inside
core/modules/system/tests/modules
which will provide form with two email fields and load this Javascript behaviour on that page and test it there? BecauseDrupal.behaviours.copyFieldValue
is part of system module.Comment #4
alexpott@msankhala it'd be better to decouple the test from the installer. I think the test should set up a form and js to do this.
Comment #5
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented@alexpott Thanks for your guide. I'll move this test to system module.
Comment #6
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is my first Drupal Functional Javascript test. I created a controller which simply return
\Drupal\Core\Installer\Form\SiteConfigureForm
and then testedDrupal.behaviors.copyFieldValue
functionality on this form.Comment #7
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedComment #8
alexpott@msankhala it'd be great to completely decouple from site configure form. We just need a form that uses the JS to test. Test is looking good. It would be good to include a test-only patch that reverts #2941106: Site email address in the install profile form is no longer copied to the user email address so we can see that the test is indeed testing the JS as promised.
Comment #9
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedThanks @alexpott for feedback. Here is new tests only patch. Now this does not uses
\Drupal\Core\Installer\Form\SiteConfigureForm
form anymore.Comment #10
alexpottLet's not base all of this around Site configure... In fact let's make this a part of the already existing
system_test
module.Let's call this CopyFieldValueTestForm.
Let's return something different here. Based on the class name
Let's not even call it configure site form. And in fact we can just simply the entire form to the two fields. They don't even need to be email fields.
When this is done the docs here should be
Extra space on the line end.
Comment #11
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHi @alexpott yes this test can completely be decoupled from site configure form. Here is new patch as per your suggestion.
Comment #12
alexpottThere is a possible timing issue here. After filling the field the JS will run async with the test. There's no helper to wait for an element value but we should wait. So we'll need to do something like:
Also let's just use a random string since any random value should be copied instead of 'admin@example.com'.
This is a tricky test. Not sure how to wait here because we are essentially waiting for nothing to happen. I think we'll have to add an explicit wait(1) and document why.
Comment #13
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented@alexpott Isn't it advised to avoid waiting for a specific number of seconds?
I tried with
waitFor
as you suggested. This caused two things.assertNotEquals
toassertEquals
then it breaks the test.This returns:
As you can see it is making 103 assertions.
In second assertion if i change
assertNotEquals
toassertEquals
like:This returns:
Comment #14
alexpott@msankhala you shouldn't do an assert in the waitFor callback. You need to assert on the result of the waiting. The callback should return something falsey to continue waiting or something truthy to stop waiting. So for example:
As noted there is nothing the second time to wait for so doing this is not going to work. Or I guess you could do:
However this code is no different to do
sleep(1)
because as stated you are testing that nothing has happened.Comment #15
alexpottAfter filling the field the JS will run async so add wait of 10s.
this is also not quite right - we are waiting for up to 10secs for the fields to be equal. Under normal conditions this will never wait that long.Comment #16
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedThanks @alexpott for detailed feedback. I missed the whole point of
waitFor
and added assertion inside the callback. Here is updated patch. Please review.Comment #17
alexpottToo many lines.
Should have some docs.
I'm not sure this is necessary.
This isn't true - don't really need a comment either.
Can just be
$page->fillField('edit-source-field', $this->randomString());
[EDIT] hmmm actually made we want the random string in a variable - see comment below.It could be worth asserting that
$target_field->getValue() == $random_string
instead since the fields both start with the same empty value.As above let's just do
return $target_field->getValue() == '';
Comment #18
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentednits: #1 removed extra lines
#2 docs added
#3 removed
#4 comment removed
#5 ...
#6 and #7 fields modified
Comment #19
Ada Hernandez CreditAttribution: Ada Hernandez at MTech, LLC commentedComment #20
alexpott$random_string = $this->randomString();
no need to create a new Random.
Comment #21
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedRemove comment.
Comment #22
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commented$random = new Random();
object.$source_field_selector
variable.Comment #23
yogen.prasad CreditAttribution: yogen.prasad commentedComment #24
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedCan anyone review this?
Comment #26
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedPatch for 8.7.x. Even though nothing changed between 8.6.x to 8.7.x.
Comment #28
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is the updated patch which fixes the failing test.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #38
karishmaamin CreditAttribution: karishmaamin at Specbee commentedComment #39
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled patch against 10.1.x. Please review
Comment #40
NivethaSubramaniyan CreditAttribution: NivethaSubramaniyan as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixing CCF in #39
Comment #41
nod_Thanks NivethaSubramaniyan unfortunately you're missing many files from the previous patch.
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #43
nod_Going with
$this->assertTrue($target_field->getValue() == $ random_string);
works.it needs to wait for the blur event on the source field, by the time $target_field->focus() is done, the blur event has already been executed (because of the order in which those event are spec'ed to occur).
Here we should probably check that the target field has the same value as before :
$random_string
that would allow us to remove the waitFor as well.Comment #44
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed points in #43
Comment #45
nod_looks good to me
Comment #46
alexpottThere's no wait here... imo we should do the same focus as in the positive test case and that should be good as I think the fill field will move the focus back to the source field
Comment #47
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo should it not wait?
Comment #48
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #49
Santosh_Verma CreditAttribution: Santosh_Verma at Srijan | A Material+ Company for Drupal India Association commentedreviewed the patch on comment #48, patch applied successfully on D10 and there is no functionality break in the website, can we merge now
Comment #50
longwaveThere's no AJAX going on here so waiting for AJAX is pointless.
Comment #51
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedUpdated patch #48 by addressing #50, please review it.
Thanks!
Comment #52
smustgrave CreditAttribution: smustgrave at Mobomo commentedEven though I worked on the ticket, this was previously marked RTBC and the update was removing 2 lines so think I can still move it back to RTBC.
Comment #54
smustgrave CreditAttribution: smustgrave at Mobomo commentedRandom failure
Comment #55
lauriiiNit: javascript should be replaced with JavaScript.
Unused variable.
This comment is redundant because that's exactly how the following line would read..
You can use
$this->assertEquals()
here 😇I'm not sure I understand this comment. Isn't the expectation that nothing happens to the target at this point? Why is this comment explaining that the JavaScript will run async?
Comment #56
Abhisheksingh27 CreditAttribution: Abhisheksingh27 at OpenSense Labs for DrupalFit commentedUploading updated #51 patch with suggested 1,2 and 3 changes in comment #55.
Comment #57
Tanuj. CreditAttribution: Tanuj. as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented#56 doesn't fix issues 1,4,5 mentioned in #55, re-uploading patch to fix all the mentioned issues, please review.
Comment #58
smustgrave CreditAttribution: smustgrave at Mobomo commented#57 appears to address the points from #55
Comment #59
lauriiiMinor adjustments to code comments.
Comment #61
smustgrave CreditAttribution: smustgrave at Mobomo commentedRandom failure.
Comment #63
nod_Committed f88d58b and pushed to 10.1.x. Thanks!