https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35287
--- Comment #58 from Victor Grousset/tuxayo <[email protected]> --- (In reply to Pedro Amorim from comment #57) > I've squashed the removal of commented cy.wait("@avcategories"); to the > squash qa follow-up patch. > I've also added sponsored-by lines. Thanks :) > (In reply to Victor Grousset/tuxayo from comment #55) > > This new file needs a /* keep tidy */ comment on top: > > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/additional-fields-api-client.js > > > > It's also a good opportunity to add a /* keep tidy */ comment on top of this > > file: > > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js > > To avoid having to manually check it with prettier. > > No api.js files under js/vue/fetch have this. JS14 guideline is more recent (march) than these. So it's case of dealing with legacy JS/TS/vue files. «Older JS files should also strive to become tidier and eventually all end up with the keep tidy header line too.» It's just that in the meantime they haven't been touched or this guideline was forgotten. > If this is reason to FQA it > should at least be flagged by the QA script first then. The point of /* keep tidy */ is to opt in the QA script enforcement of JS14. That's how to deal with the old files that are totally not compliant. (so not checked by default) So until they are pretty and /* keep tidy */ is added, they will never show up in the QA script. At least not for this check. > > there are two > > cy.get("#licenses_list table tbody tr:first td:first a").click(); > > > > IMHO either they should get a comment to easily know what's clicked when in > > the future there will be some change to the structure of the table and the > > test will break. > > Or to prevent breakage and documenting at the same time, an id or a class > > could be added to what's being clicked (or it's parent) so the selector > > speaks for itself and would be much less likely to break with future > > changes. > > This is an established pattern utilized in all cypress spec files to click > on the first element of a table (to navigate to it). Oops, sorry, the current selector is actually fine. I guess after reading too much in a row from these large patches my brain got fried 😵💫 > I'm on board for filing a new bug > titled "Add id to table rows to be more easily selected by cypress tests" > but I don't see how this should be in the scope of this patchset. Even if not a relevant case here, about the general idea: No need to fill a separate bug. It can totally be part of adding a test. I know it can feel wrong to even change minimally the markup to accommodate a test. But I think it's only because tests can still feel like second class citizens. We can think of it like when extracting a method to be able to unit test it. There it's okay to do so at the same time as adding tests for changes; make the code more easily testable. So it should be ok (when the change is easy to make at least) to make the markup more testable. Both to get more reliable tests for if the page structure change. And have more readable tests and the markup itself. As a fallback, having a comment to not have an undocumented "magic selector" (cf. magic number) (again not at all needed for this case, it's fine) -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
