Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
There is no way to get the drupalSettings
as part of the test suite, unlike with the old test suite.
Source: Jaesin
Proposed resolution
Add it back, have a different implementation for JS vs. no JS
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#30 | 2787923-30.patch | 2.71 KB | claudiu.cristea |
#30 | interdiff.txt | 634 bytes | claudiu.cristea |
#31 | diff-8.3.x-8.2.x.txt | 690 bytes | claudiu.cristea |
#31 | 2787923-31-8.2.x.patch | 2.7 KB | claudiu.cristea |
Comments
Comment #2
dawehnerComment #3
neclimdulHard problem number 2, being as we have different types of settings in Drupal it seems like this should document that this is getting javascript
drupalSettings
. I couldn't figure out why only JTB cared about Drupal's settings and only after reading the implementation did it make sense. Preferably in the signature I think but something.Comment #4
dawehnerWell, BTB has its own variant already, but its not able to take into account he dynamicness of JTB.
Comment #5
claudiu.cristeaAdded test.
Comment #6
claudiu.cristea@dawehner, this is tested now in #5.
Comment #7
dawehnerNice @claudiu.cristea
I wouldn't have expected someone to jump in that quick. Awesome!
Comment #8
claudiu.cristea@dawehner I was more or less cross-posting. I did the patch before your comment in #4
Comment #9
Jaesin CreditAttribution: Jaesin at Chapter Three commentedI was using this before I knew I could execute JS:
Comment #10
claudiu.cristea@Jaesin, a variant of that is now implemented in BrowserTestBase. But that cannot resolve the dynamic case from the test in #5.
Comment #11
dawehnerYeah I think this code is just in 8.2.x but not in 8.1.x
Comment #12
claudiu.cristeaComment #13
dawehnerIs there any reason we need to less readable JS here? We could also use new lines I guess, right and by doing so, make it more readable.
Comment #14
claudiu.cristeaComment #15
dawehnerThanks a lot @claudiu.cristea!
<3
Nitpick: This line seems to have one space too much :)
Comment #16
neclimdulCould we mention something about these being Javascript settings? It sounds like they're coming from _SESSION or something when really that's referring to the internal working of mink not what you're retrieving. IE You have to know this method lives on JTB to understand the documentation which isn't really great.
Comment #17
claudiu.cristeaComment #18
neclimdulawesome!
Comment #20
claudiu.cristeahttps://www.drupal.org/pift-ci-job/519413 failure is weird. Trying to rerun the test.
Comment #21
cilefen CreditAttribution: cilefen as a volunteer commentedComment #22
claudiu.cristeaVery small docs fix.
Comment #23
klausiIf a test method requires an additional module then this is a good sign that the test method should live on its own test class instead. We should stop accessing the Drupal API altogether in functional tests like that, see also #2783193: Make browser tests use the browser only for assertions.
Leaving at RTBC for now, not sure if that should hold up this patch.
Comment #24
catchHmm I think it's worth holding the patch up to be honest. We don't have many js tests yet and most people will add to them by copy/paste/modify, so should try to keep what we have as tidy as possible.
Comment #25
claudiu.cristea@catch, you mean separate the test in its own class or add the module in BrowserWithJavascriptTest::$modules?
Comment #26
catch@claudiu.cristea I don't really mind on that. For test performance a new class is a bit better since we won't install the module unnecessarily for the other methods, so maybe new class?
Comment #27
claudiu.cristeaHere we go.
Comment #28
dawehnerNice work
Comment #30
claudiu.cristeaOuch! the namespace. Setting back to RTBC as per #28.
Comment #31
claudiu.cristeaMain patch in #30. Adding also a patch for 8.2.x if will ever be considered.
Comment #33
catchCommitted/pushed to 8.3.x.
Leaving RTBC against 8.2.x - tests still running for a start.
Comment #34
claudiu.cristea8.2.x test is green too.
Comment #35
alexpottCommitted 13b6d5f and pushed to 8.2.x. Thanks!
Cheery-picked to 8.2.x cause this makes 8.3.x and 8.2.x test environments the same.
Comment #38
alexpottOops there was an 8.2.x specific patch... the modules visibilty how silly. Committed 5d2a732 and pushed to 8.2.x. Thanks!
Comment #39
claudiu.cristeaComment #42
claudiu.cristeaComment #43
claudiu.cristea