-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor map test #15000
refactor map test #15000
Conversation
jenkins, test this |
compareTableData(expectedZoom5Data, actualZoom5Data.trim().split('\n')); | ||
// await PageObjects.visualize.selectTableInSpyPaneSelect(); | ||
// const actualZoom5Data = await PageObjects.visualize.getDataTableData(); | ||
// compareTableData(expectedZoom5Data, actualZoom5Data.trim().split('\n')); | ||
|
||
await PageObjects.visualize.closeSpyPanel(); | ||
await PageObjects.visualize.clickMapZoomIn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guessing these lines are important to keep in for the next test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. it keeps zooming in. there's some mistake still..
Rearranged test for clarity. the default styling seemed to have changed a little bit, causing little shifts of the bounding box, causing little changes in the data we get back. |
jenkins, test this unit test failed on license, but not related to these changes |
jenkins, test this one last time |
]; | ||
it('zoom in to top level, check required behaviors on the way dwn', async function () { | ||
|
||
// const expectedZoom5Data = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you link to the issue again and explain about why it's all commented out? Unless i don't see it, I think you had that in here before.
So the sort and prepData fixed the other functions but still not this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll llnk the issue again.
the prepData is just to simplify the code a little, while I was working on it. but those resolved errors i got locally, and have never seen in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm ... ok this is a bit confusing... if we are disabling a test, this pr should just remove the test in question, but it seems it:
- changes behaviour of a test
- changes description of another test
- comments out part of the test
i think it should just remove the test (we use git to track history, no need to keep commented out parts of the code in there) or if we are just removing the part of the test, remove that part, and make it clear in PR description that this is just removing a part of test.
@ppisljar makes sense, wasn't clear enough that this is WIP still. PR sort of started to bloat. sorry about the confusion. updated title for clarity |
might addres #14920
Can't reproduce this locally, but seems to fail in CI. Let's disable for now since it's holding up PRs.
I also started to refactor the test. Right now, as the last test-cases are dependent on each other, it is not good to keep them in different stubs.