test(cypress): notification history tests #2816
Conversation
|
| cy.writeLPData({lines: [lp], offset: offset, namedBucket: '_monitoring'}) | ||
| } | ||
|
|
||
| beforeEach('Generate alert records', () => { |
There was a problem hiding this comment.
I've seen flaky tests in the past from beforeEach functions that don't handle async functions & promises. I think we should refactor this to use cypress chaining (cy.xyz().then(() => cy.abc().then(() => ...) to make sure the beforeEach doesn't finish prematurely.
There was a problem hiding this comment.
Generalized this to a localInit() method that returns CypressChainable. It's at the start of the chain. It handles a lot of local data. It is probably too specific to warrant being generalized into a command.
There was a problem hiding this comment.
Cool, looks much better. You could move it back into a beforeEach to avoid having to call it in each test. As long as the code within the beforeEach returns a Cypress chainable object, it will wait for completion before starting the test. But this implementation is fine too.
| } | ||
| }) | ||
| // verify first row | ||
| cy.get('[class$=ScrollContainer] > div:nth-of-type(1)').within(() => { |
There was a problem hiding this comment.
is there a reason we aren't using testIDs here? I find being dependent on CSS class names to be an issue because those change pretty often. We're working on a pretty major overhaul of Clockface which will probably change most CSS class names and this test will have to be completely re-written. If we use testIDs, we don't have that issue
There was a problem hiding this comment.
I agree, data-testid values are preferable because theoretically less prone to change. However, when investigating the Check Statuses page, I could find no data-testid attributes below data-testid='page-contents'. Coming from QA, I'm hesitant to modify code in the application under test, in other words to insert data-testid attributes into the page. However, if you have no problem with this, I'll go ahead and do it and then modify the test code accordingly.
There was a problem hiding this comment.
Yeah I think we should add the testID attributes if you don't mind. Most of the Clockface components come with generic testIDs for free. But if one is missing, adding it to the component here in the UI repo is the right move. Sorry to suggest so much refactoring...but I know this Clockface overhaul is coming soon and a lot of these CSS classes will change so it's important that we use testIDs for new tests
| // ...To Check | ||
| cy.getByTestID('overlay').should('not.exist') | ||
| cy.get( | ||
| '[class$=ScrollContainer] > div:nth-of-type(4) .event-row--field:nth-of-type(3) > a' |
There was a problem hiding this comment.
this is a great example where a testID should be used. It's really hard to tell what you're trying to click on here just by reading through the test code
There was a problem hiding this comment.
Agreed. But will need to add the data-testid attributes myself. See response to comment on line 545.
| rec.statusTimestamp | ||
| },_message="${rec.message.replace(/ /g, ' ')}",dead="true"` | ||
|
|
||
| cy.writeLPData({lines: [lp], offset: offset, namedBucket: '_monitoring'}) |
There was a problem hiding this comment.
change to return cy.writeLPData(...). because cy.writeLPData is async, we need to return the chainable object here to handle the promise correctly
| }, | ||
| '20m' | ||
| ) | ||
| writeMonitoringRecord( |
There was a problem hiding this comment.
this function is technically async because cy.writeLPData is async. So we need to chain off of it like writeMonitoringRecord(...).then(() => ...
| // N.B. if local === utc then skip | ||
| if (new Date().getTimezoneOffset() !== 0) { |
There was a problem hiding this comment.
instead of this we could modify package.json scripts to set the TZ variable like this: cypress-io/cypress#1043 (comment)
There was a problem hiding this comment.
Applied this change. First run of monitor-ci failed on the local to UTC switch test. Second run passed. So, I'm a bit suspicious of whether this will be a stable change.
There was a problem hiding this comment.
Ah rats ok. We have a new ability in our pipeline to run tests 20x so we could do that to check for flakiness before merging.
|
@karel-rehor I did some work today to get the pipeline to set the TZ variable. I merged everything in so you should be good to try and run this again. I also think we should probably undo the changes to package.json so that developers can experience the tests running in their local timezone when running locally. I was able to set the TZ variable in the pipeline definition itself, outside of this configuration. Let me know if it doesn't work. Thanks! |
|
@genehynson - I've rerun this five or six times today and each run has passed. I've just restored the beforeEach block and removed the localInit() function, which felt a bit of a hack and an anti-pattern. It looks like The TZ variable in the pipeline works. It has been removed from scripts in |
* chore: update create check for e2e test * test: start cypress notification history tests * test: add asserts, and refresh notification history test * test: tidy new notification history test * test: improve chaining of async calls in notification tests and set timezone for tests * test: add data test ids to event table * chore: lint and prettify recently changed files * chore: remove TZ argument from package.json scripts * chore: restore beforeEach with return val
Adds tests of the Notification history page accessible from a Notification Rule
_monitoringbucket.