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

[open-formulieren/open-forms#3611] Make time bounds validation inclusive #596

Merged
merged 4 commits into from
Nov 22, 2023

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Nov 20, 2023

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (7aee061) 72.00% compared to head (3598e1a) 72.79%.
Report is 4 commits behind head on main.

Files Patch % Lines
src/formio/validators/MinMaxTimeValidator.js 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
+ Coverage   72.00%   72.79%   +0.78%     
==========================================
  Files         213      213              
  Lines        4319     4323       +4     
  Branches     1152     1155       +3     
==========================================
+ Hits         3110     3147      +37     
+ Misses       1162     1134      -28     
+ Partials       47       42       -5     

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

Copy link
Contributor

@SilviaAmAm SilviaAmAm left a comment

Choose a reason for hiding this comment

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

Does it need to be rebased on main to fix failing tests?

@Viicos Viicos force-pushed the issue/3611-inclusive branch from b4f1e4e to d245e80 Compare November 21, 2023 10:31
@Viicos
Copy link
Contributor Author

Viicos commented Nov 21, 2023

Does it need to be rebased on main to fix failing tests?

Weird, tests pass locally (before and after rebase). And the failing tests are from the hash based routing PR

@sergei-maertens
Copy link
Member

Does it need to be rebased on main to fix failing tests?

Weird, tests pass locally (before and after rebase). And the failing tests are from the hash based routing PR

I suspect a race condition in the redirect flow vs. test assertion time, I had similar suspicious flaky local failures too in another branch.

@Viicos
Copy link
Contributor Author

Viicos commented Nov 21, 2023

vs. test assertion time

By "time" do you mean the tests related to the time validation? or in the literal sense

@sergei-maertens
Copy link
Member

vs. test assertion time

By "time" do you mean the tests related to the time validation? or in the literal sense

The point in physical time when the test assertion line executes - I suspect that the expect(...) call is invoked/scheduled before the redirects have fully completed and thus window.location is not updated yet or does not reflect the updated state yet.

Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Let's aim to fix the test race conditions/flakiness separately, however this test refactor needs to either call the done callback or use await sleep(300) for the delays and then make the assertions.

Comment on lines 31 to 38
setTimeout(() => {
if (valid) {
expect(!!component.error).toBeFalsy();
} else {
expect(!!component.error).toBeTruthy();
expect(component.error.message).toEqual('invalid_time');
}
}, 300);
Copy link
Member

Choose a reason for hiding this comment

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

this should also be made async, I think we have a sleep utility somewhere. Now the end of test execution is never signalled and the block likely exits before the assertions are made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, feels really weird from a UX perspective. I guess this is the current state of JS frontend tests

Copy link
Member

Choose a reason for hiding this comment

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

no, it has everything to do with Formio's event dispatching and debouncing. With a pure react implementation, we shouldn't need setTimeout anymore

@sergei-maertens sergei-maertens added the needs-backport Fix must be backported to stable release branch label Nov 21, 2023
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Test flakiness was addressed in #599

@sergei-maertens sergei-maertens merged commit 80ad3c0 into main Nov 22, 2023
14 of 15 checks passed
@sergei-maertens sergei-maertens deleted the issue/3611-inclusive branch November 22, 2023 11:16
sergei-maertens pushed a commit that referenced this pull request Nov 22, 2023
@sergei-maertens
Copy link
Member

Backported in 41138f2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-backport Fix must be backported to stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time validation: When max is set by user causes warning.
3 participants