Integration Tests (Finally) #92
Conversation
What started as a simple "Let's see what jsHint issues arise" made me find a couple things. One section was missing semi-colons, which I added (which is just a styling change). After fixing globals and the semi-colons, I found a section that was referencing a variable that wasn't actually set. I saw in a similar test created it before starting the increment, which I blindly copied into the test. Tests passed before and after the changes. These are all styling changes.
So this adds a few tests using the child_process.fork() method, in the attempt to recreate starting/ restarting conditions that may occur for a service or application. These have been broken into each job assignment method. .each() - Ensures jobs do not get re-ran after restart. - Ensures array names still have jobs ran. .schedule() - Ensures on startup future jobs do not get ran. - Ensures jobs scheduled before current time get ran. - Ensures array names still have jobs ran. .every() - Ensures job gets ran as soon as possible. There are a few nitpicks here that I still have, such as using path.join() to ensure this can be tested in non-Unix enviroments. Seeing as this uses Make, I am not 100% sure how useful cross-compatibility is, not to mention I haven't validated that the rest of the codebase uses path.join() either.
|
Sorry, had noticed a dumb IIFE in the |
|
I'll take a look at this when I get home, no idea how removing an IIFE reduced test coverage. |
|
@droppedonjapan This is fantastic! Thank you for the work! Feel free to open another PR if you figure out the issue w/ coveralls -- otherwise, I'm not too worried. Processing w/ multiple agenda instances could be interesting -- my guess is that we would find improvements for potential load balancing across instances, although improving it w/o more robust messaging between instances might not be possible. |
|
@rschmukler Yeah, that could be a fun project to work on, if nothing else to have some sort of benchmarks. I wonder, how does one benchmark that sort of thing? Since users will be doing their own random jobs, I'd imagine you could mock up longer and shorter processes with different timeouts, maybe get a timeline of what jobs Agenda is working on at what time. If you get a simple array of start / end times for jobs, you could graph it with some random charting library. I guess the real question is if a clustered setup for tasks would be an eventual goal to support out of the box? |
So if you look at this PR, you might be amused to see that I stuck with the
child_process.fork()method for testing here.Initially I had tried invalidating the
new Agenda()instance and recreating it, however that led to a lot of odd problems that I couldn't fully figure out, so I scrapped that idea in favor of a way of dynamically loading Agenda jobs in the forked process.A few caveats with this PR.
path.join()to ensure cross-compatibility with non-Unix environments. That being said, I haven't through the rest of the code yet to know if that's true (and the ability to useMakeon a Windows machine is questionable).var counter = 0;statement to match the surrounding tests. This was a total blindly made change, but the tests passed before and after I made the changes, so I think we're good!If you want these commits squashed, let me know and I can take care of it.
Do you have any other ideas for integration tests? I thought we might be able to add something to check manual job processing and concurrent Agenda instances, but I'm not super-familiar with that API and figured we could start with these.