SlateFoundation / slate-cbl

http://slatefoundation.github.io/slate-cbl/
MIT License
7 stars 7 forks source link

feat(e2e): add test cases for overrides [SCHOOL-125] #703

Closed themightychris closed 1 year ago

themightychris commented 2 years ago

Closes: SCHOOL-125

Note on tests: You may want me to move these tests to existing files. There are 3 functions in this file. One, just a simple loadDashboard() is from teacher-dashboard.js, another is the getSlidersRatingSelector() from teacher-submit.js and a third overrideSkill() that is new. The last test is the only test that uses getSlidersRatingSelector() and it does not use overrideSkill(). I think a potential way to split these up is to put the first 7 tests along with the overrideSkill() function into teacher-dashboard.js and move the last test into teacher-submit.js.

themightychris commented 1 year ago

@BillClinton see changes I started making it c7f2485—you should use const whenever possible instead of letlet implies that you expect the variable to be assigned a new value after initially defined

And then also to use the named variables in place of repeating its values as magic numbers later

Also better to use template literals these days instead of string concatenation like you did lower down with '...'+student+'...'

themightychris commented 1 year ago

@BillClinton I made some changes in d58832a99 that I think are necessary to the tests stability. In the test suite where you pulled loadDashboard it didn't take a competencyArea/studentsList hashPath in most cases, and then in the cases where one got passed the calling test had some extra assertions to wait for the dashboard to load

In here, we didn't have anything waiting for the dashboard to load. I modified the signature of loadDashboard here since we pass a competencyArea+studentsList in every case to make that mandatory, and then added a built-in wait to this suite's version of loadDashboard

themightychris commented 1 year ago

@BillClinton I found the biggest instability issue in this test suite in a4168593d81628ff15bea29a01f787a4e72d9fe0

You cleverly used cy.extGet('slate-window title[text="Submit Evidence"]') to target specific windows by their title, but this had the effect also of shifting the returned subject of cy.extGet from the window to the title component, which then defeated the .should('not.have.class', 'x-hidden-clip') technique for waiting for an ExtJS window to finish rendering—so the following assertions would run immediately without waiting for the window to render and fail to find elements within it if it didn't render fast enough. I was able to fix this via ComponentQuery's syntax for matching E ^ F All parent Components of E that match F and just flip all the syntaxes to the likes of cy.extGet('title[text="Submit Evidence"] ^ slate-window') so we'd select windows based on title but leave the window as the subject

The .within() code worked even though the subject was the window title which didn't contain any of the elements we cared about because cy.extGet is always global and not subject to .within()'s scoping. We want to use regular CSS selectors with cy.get() when using .within() and stay entirely within DOM-land in there. I fixed that in 2374bb603f742a53c11eb34096eefcdcd5308eae and also dropped typing in comments for each override since typing is a slow and error-prone operation and in this case doing it doesn't lend anything to the test since we never assert on it. It might make sense to have one spot where we type in an override comment and then assert that gets saved and shows up in the right spot. Generally in cypress tests you want to focus on the thing you're testing and do all the quickest/simplest/safest ways to get there—that's why we use an XHR call to login ahead of every test instead of using the login form each test and then just have one test of the login form itself that actually uses the form