Respect query strings (cachebusting) - #118
Conversation
ac59d46 to
0861560
Compare
cadic
left a comment
There was a problem hiding this comment.
@peterwilsoncc Nice job with tests!
I have just one concern about the initial state before assertions. When you testing the cache-busting scenario, you expect that the database already has a correct record set up. It works in this particular situation because previous tests have saved the correct record. But you made your tests rely on other tests results https://docs.cypress.io/guides/references/best-practices#Having-tests-rely-on-the-state-of-previous-tests
So if someone will insert a new test just before yours (which is OK), and their test will save some data that your test doesn't expect (which is OK too)... Your test will fail and this is not a responsibility of that person. So it will be great to save the database record in the beginning of your test.
|
@cadic Thanks for the review. I ended up combining the querystring tests with the previous test to avoid the need to repeat setting up the code. There's a section on the best practices guide pointing out the difference between E2E and unit tests. It argues for grouping similar assertions. |
cadic
left a comment
There was a problem hiding this comment.
Thanks for the updates @peterwilsoncc
Description of the Change
Displays the
ads.txtandapp-ads.txtfiles for any URL beginning with(app-)ads.txt?-- this is to account for requests that include a querystring.The value of the query string has no affect to emulate the affect of hitting an actual text file.
Closes #26.
How to test the Change
Changelog Entry
Credits
Props @helen, @peterwilsoncc.
Checklist: