-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Reduce JS test warnings #28772
Reduce JS test warnings #28772
Conversation
Remove "ERROR: undefined" at the end of the tests which were due to the "onbeforeunload" event. Removed "download.svg" errors. Removed warnings about missing plurals.
for the remaining parts I raised #28773. |
core/js/tests/specs/coreSpec.js
Outdated
OC.Notification.hide(); | ||
|
||
expect(warnStub.calledOnce).toEqual(true); |
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.
I don't think this is a good approach. Expecting that a warning message will popup doesn't feel good.
The warning message shouldn't popup in the first place, so we should try to fix the cause of the warning first. In addition, we shouldn't expect that the warn method will be called because it isn't essential for the test. As we don't check a "log" method of any logger will be called (thinking in PHP), we shouldn't check if a "warn" method will be called.
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.
The cause of the warning is that some code out there (users page) is still using this method without arguments.
The expectation of this test is that a warning does appear, so I think it's ok here.
If this bothers you then I should probably remove this test completely.
But the point of the test was also to make sure that hiding still works even if the arg is missing.
Or if you like I can just remove the expectation but keep the method stubbed to avoid output.
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.
Or if you like I can just remove the expectation but keep the method stubbed to avoid output.
I think it's this (with a comment to explain why this is stubbed), or make the code not to use the warn method.
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.
added comment df92098#diff-163902768f6bb1fc76108ce0e0c7cd5eR997
0cf3c47
to
df92098
Compare
I'd definitely remove the expectations for the warning message in the console: they're not part of what we want to test. What we want to test is the global behaviour: you call |
df92098
to
8586152
Compare
@jvillafanez I've removed the "expect()" block |
👍 |
…ings Reduce JS test warnings
…034eca680160791808cd [stable10] Merge pull request #28772 from owncloud/jstest-reduce-warn…
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
Remove "ERROR: undefined" at the end of the tests which were due to the
"onbeforeunload" event.
Removed "download.svg" errors.
Removed warnings about missing plurals.
For the rest, need to spend some time to replace the last tipsy usages and port some of the notification calls.
Related Issue
None raised
Motivation and Context
Warnings are annoying.
How Has This Been Tested?
make test-js
See that there are less warnings.
Screenshots (if appropriate):
Types of changes
Checklist: