Skip to content
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

Fix acceptance tests with new v-calendar #8151

Closed
dschmidt opened this issue Dec 23, 2022 · 13 comments
Closed

Fix acceptance tests with new v-calendar #8151

dschmidt opened this issue Dec 23, 2022 · 13 comments
Assignees
Labels

Comments

@dschmidt
Copy link
Member

In #8128 we updated to a new major version of v-calendar and while it seems to work just fine when testing it manually, the acceptance tests are broken - so we skipped them for now.

It would be great if the QA Team could properly fix it.
Thanks in advance!

See commit "Skip expiration date picker acceptance test as it's broken with the n…" - currently it is 932c408 here but we are rebasing constantly, so possibly not for long.

@phil-davis
Copy link
Contributor

@amrita-shrestha @SagarGi please get someone to sort this out.

@dschmidt
Copy link
Member Author

dschmidt commented Dec 23, 2022

We made the OcDatepicker component instance available on the DOM Element to be able to fix the move invocation, (as you can see in the referenced commit and the one before) but to be honest I would prefer if we could get rid of that and internal api usage all together.

As it's used in one acceptance test only, it shouldn't be too much of a performance problem if we just click in the UI I hope. Moreover we fixed the infamous December issue recently, which should be included in the v calendar version used in the PR, so we can get rid of that extra code path as well

@dschmidt
Copy link
Member Author

There's also an e2e Test which uses the same more or less internal api, up to you but it might be worth porting the acceptance test over and fixing it just once

@saw-jan
Copy link
Member

saw-jan commented Dec 27, 2022

The problem with the failing acceptance test webUISharingPublicExpire/shareByPublicLinkExpiringLinks.feature:11 is that whenever the test tries to click the year 2038, it will click the year 2035 (just above it). This is due to the scrolling-back issue that we have raised earlier here #7513

The proper fix for the date picker tests would be to fix the auto-scrolling issue of the calendar

CC @dschmidt

@saw-jan
Copy link
Member

saw-jan commented Dec 27, 2022

Changing the test code will only make the tests pass for a certain period of time.
The tests should work nicely with the new v-calendar if that scrolling issue is fixed soon. Should we close this issue here?
Or any thoughts? @dschmidt

@saw-jan
Copy link
Member

saw-jan commented Dec 27, 2022

I will post the current (master) and new (vue3-compat-ng) scrolling behavior of the calendar.
I see some difference

@dschmidt
Copy link
Member Author

Ok, cool.

If it's just a test issue I'd be happy if we could get a temporary fix which won't make the scope of the PR much bigger, as it's really hard to review already

@dschmidt
Copy link
Member Author

Let's fix it properly in a follow up maybe?

@saw-jan
Copy link
Member

saw-jan commented Dec 27, 2022

I will post the current (master) and new (vue3-compat-ng) scrolling behavior of the calendar.
I see some difference

I think the behavior is the same on both. The only difference is the calendar size due to which the test is passing on master but not in vue3-compat-ng branch.

Master:

simplescreenrecorder-2022-12-27_18.08.42.mp4

vue3-compat-ng:

simplescreenrecorder-2022-12-27_18.06.35.mp4

If it's just a test issue I'd be happy if we could get a temporary fix which won't make the scope of the PR much bigger, as it's really hard to review already

Maybe we can adjust the date as of now.

CC @dschmidt @phil-davis

@saw-jan
Copy link
Member

saw-jan commented Dec 28, 2022

Maybe we can adjust the date as of now.

But we also want to be sure that the issue with auto-scroll is caught

@phil-davis
Copy link
Contributor

Maybe we can adjust the date as of now.

yes, adjust the future date to some closer date that does not cause this known date-picker problem. The test scenario is just testing that the expiration date of a public link share can be adjusted.

The dates currently chosen are after the date for the https://en.wikipedia.org/wiki/Year_2038_problem and so they are checking that the software does not have this future problem. But I don't think that we need to be doing that specifically in a test like this. We can easily add specific "year 2038" test scenarios as a separate thing some time in the future.

@saw-jan
Copy link
Member

saw-jan commented Dec 29, 2022

Can this be closed now?
CC @phil-davis @dschmidt

@phil-davis
Copy link
Contributor

Yes - closing. The "year 2038" is a different thing, and we are some way off from rushing to test that everywhere just yet!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants