Finish tests for reading lists service #1055
Conversation
Bug: T184545
1) Making all tests work and removing outdated order endpoints 2) Adding test cases for batches endpoints Bug: T184545
test/features/lists.js
Outdated
| if (!req.href.startsWith(`${config.baseURL}/data/lists/`)) { | ||
| throw Error(`Unmocked request to ${req.href}`); | ||
| } | ||
| })); |
There was a problem hiding this comment.
Judging by massive Travis failures this is not a great approach. The nock interceptor once installed never goes anywhere, so you need to manually remove this listener.
Removing nock event after the the reading lists test is complete Bug: T184545
test/features/lists.js
Outdated
| }); | ||
|
|
||
| after(function() { | ||
| nock.emitter.removeAllListeners('no match'); |
There was a problem hiding this comment.
Hm.... Don't think this is a good idea either. What if other test suites or nock internals have set something.. Let's store the callback function we set in a variable and use removeListener?
There was a problem hiding this comment.
@Pchelolo I tried that and kept the same error as before, maybe I did it wrong somehow, I will try it again. Also, I am not sure if it is necessary to check for unmocked requests on no match events, there is no request besides /data/lists/* and to the mw api. What do you think?
There was a problem hiding this comment.
Ouch, clicked the wrong button..
Hm... Not sure why wouldn't it work, it should if you're doing it like in the doc.. https://nodejs.org/api/events.html#events_emitter_removelistener_eventname_listener
In general, I'm not sure it worths bothering with it - you are mocking en.wikipedia.org domain and all requests go to it, and for mocked domains unmocked requests will throw anyway. Something really wrong should happen for us to start talking to some unmocked domain.
To some up, if removeListener doesn't work, no need to bother with this IMHO, just remove the whole thing
There was a problem hiding this comment.
Ah, nevermind I was still using an anonymous function, my mistake. New commit on the way.
Changing removeAllListeners to removeListener Bug: T184545
|
Ok, travis's happy - good. I didn't really look closely into all the individual mocks, but I guess they're fine. In the meantime, can you We don't really lint the tests all the time, but it's better if new tests had less linting errors. |
Skiping console-log rules in one line
|
@Pchelolo thank you for reviewing it. Do you think is a good idea adding the linting check to a pre-commit hook or something like it? |
|
The fail is related to |
|
If there are timeout problems, they are at least transient. I just restarted the CI jobs a couple of times; the first timed out before the test suite started, and the second run passed. |
|
LGTM 👍 |
We do have linting running as a part of the test suite, but we don't lint tests - mainly because neither of us have time to actually re-write the tests so that they pass linting :) My request for linting for you was driven by the desire not to introduce tech debt with the new tests. |
|
Did you run the lint on the new tests? How did it go? |
Oh, ok. I understand.
Yes, after it I fixed over 100 lint issues (mostly indentation), it now has no lint issues for |
😘 |
Continuing from @tgr PR
Bug: T184545