Skip to content

Conversation

@kavilla
Copy link
Member

@kavilla kavilla commented Mar 4, 2025

Description

Caniuse out of date cause one of our tests to fail because it does a raw compare and sees the caniuse complaint.

Also, returning back a promise and then doing stuff in the save search spec.

Also just bumping up the timeout for the get elements just to give it more of a chance.

Issues Resolved

n/a

Screenshot

n/a

Testing the changes

#9466

Changelog

  • test: update caniuse and return promise and properly chain the async operations

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Caniuse out of date cause one of our tests to fail because it does a raw
compare and sees the caniuse complaint.

Also, returning back a promise and then doing stuff in the save search
spec.

Also just bumping up the timeout for the get elements just to give it
more of a chance.

Issue:
n/a

Signed-off-by: Kawika Avilla <[email protected]>
@codecov
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.75%. Comparing base (9ecb66d) to head (9502946).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9483      +/-   ##
==========================================
+ Coverage   61.74%   61.75%   +0.01%     
==========================================
  Files        3816     3816              
  Lines       91885    91885              
  Branches    14564    14564              
==========================================
+ Hits        56735    56745      +10     
+ Misses      31493    31482      -11     
- Partials     3657     3658       +1     
Flag Coverage Δ
Linux_1 28.98% <ø> (ø)
Linux_2 56.41% <ø> (ø)
Linux_3 39.26% <ø> (-0.01%) ⬇️
Linux_4 28.87% <ø> (ø)
Windows_1 29.01% <ø> (+0.01%) ⬆️
Windows_2 56.36% <ø> (ø)
Windows_3 39.27% <ø> (ø)
Windows_4 28.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

return postRequestSaveSearch(config).then(() => {
updateSavedSearchAndSaveAndVerify(config, workspaceName, DATASOURCE_NAME, true);
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this change needed? Your description says something about bumping the timeout, but is this section related to that?

baseUrl: 'http://localhost:5601',
specPattern: 'cypress/integration/**/*.spec.{js,jsx,ts,tsx}',
testIsolation: false,
testIsolation: true,
Copy link
Collaborator

@angle943 angle943 Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously i was wondering why this was set to false. If tests pass then yeah this should be set to true.

But to give some background knowledge. In other environments we set this to false. This is because some of our tests require authentication and that does not persist if this is set to true

return cy.get('@INDEX_PATTERN_ID').then((indexPatternId) => {
// POST a saved search
cy.request({
return cy.request({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why returns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wasn't originally seeing things wait for the saved search to load which should be async so i'm returning the promise chain so that then we can chain off the response and then proceed to validate. but i could be missing a wait from intercepting somehting.

});
Cypress.Commands.add(
'getElementByTestIdLike',
(testId, options = { timeout: Cypress.config('defaultCommandTimeout') }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be a way to just type the options so that it has all the things that the usual options for cy.get() has. This way if we ever need to add more stuff in the future, we don't need to explictely update the typing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you already set this value at the cypress config level so i was kind of shooting in the dark with this one and can prolly remove not positive if the option will be passed down nested levels would make sense to me. but basically anyone could override it. unless you are trying to say it would be better to have a set an options that we would let people set and not let them configure it past that

Signed-off-by: Kawika Avilla <[email protected]>
Comment on lines +19 to +20
viewportWidth: 1920,
viewportHeight: 1080,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any screenshot comparisons we have to worry about breaking here?

// using a POST request to create a saved search to load
postRequestSaveSearch(config);
updateSavedSearchAndSaveAndVerify(config, workspaceName, DATASOURCE_NAME, false);
cy.wrap(null).then(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to ensure that the async operations are completed before continuing to the assertions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants