| eydaimon | would "respawn limit 0 5" mean attempting to respawn indefiniately every 5 seconds, or does it simlpy mean it will not try to respawn at all? (COUNT=0) ? | 00:17 |
|---|---|---|
| jodh | xnox: I've now got a fix that works for https://code.launchpad.net/~jamesodhunt/upstart/bug-1199778/+merge/174138. Now just need to sort out the new test. Current plan is to cut down my new json test to say 3 session jobs, pre-process foo.json.in into foo.json (which will expand @CHROOT_PATH@ in the .in file), create an actual @CHROOT_PATH@ directory somewhere, put 3 jobs into that path, then pass @CHROOT_PATH@ to test_state. | 15:33 |
| jodh | xnox: to be clear, @CHROOT_PATH@ won't be an actual chroot, it will just be a directory containing @CHROOT_PATH@/etc/init/job[1-3].conf. | 15:34 |
| xnox | jodh: i didn't think we need to create @CHROOT_PATH@/etc/init/job[1-3].conf. nor process the json at all. | 15:34 |
| jodh | xnox: my new test found a bug in the branch, but only because I had jobs in "@CHROOT_PATH@". | 15:35 |
| xnox | jodh: load json file up, in memory, substitue chroot_path to a mktemp generated one (just the name, don't actually create it), and test against that. | 15:35 |
| jodh | xnox: can do, but we still need the jobs on disk. | 15:36 |
| xnox | jodh: that's not unit-testing any more. | 15:36 |
| xnox | jodh: where is your fix? "that works for https://code.launchpad.net/~jamesodhunt/upstart/bug-1199778/+merge/174138" | 15:37 |
| jodh | xnox: maybe not, but the point is we *should* test this behaviour since you test failed to detect a problem. | 15:38 |
| xnox | or what you are saying to reproduce the bug you need the chroot's conf-sources to be present on disk, that's fine. | 15:38 |
| xnox | i thought we agreed to remove re-loading conf-sources whilst de-serialising. | 15:39 |
| xnox | thus above test with a full chroot session present, is testing the old behaviour instead of the new one. | 15:40 |
| xnox | there shouldn't be any job_classes loaded up at that point, and the goto error, is correct in that place. | 15:42 |
| xnox | (for sessions) | 15:42 |
| jodh | xnox: my current changes do deserialise conf sources *with* sessions, but do not create conf sources *from* sessions. I'll push it somewhere but if you look at the code, other parts make assumptions and this is the minimal change to avoid major test-code rework. | 15:42 |
| xnox | jodh: yeah, please push your code. cause you are asking me if above is a valid test for code I haven't seen yet =) | 15:43 |
| jodh | xnox: I'm just trying to explain the context of what I'm proposing here. | 15:44 |
| jodh | xnox: one sec... | 15:44 |
| jodh | xnox: lp:~jamesodhunt/upstart/bug-1199778-16july2013 | 15:48 |
| xnox | jodh: the bug is that session_deserialise_all produced too much objects, when deserialised and it's conf-sources are present. So, I think an additional test around session_deserialise needs to be added that (1) create temp chroot path with a single job (2) creates chroot session for that path (3) tests that there are no job classes (4) serialise, clean environment, deserialise (5) check that there are still no job clases. | 16:10 |
| xnox | this small unit test, should fail, until "- session_create_conf_source (session, TRUE);" patch is applied. | 16:11 |
| xnox | similarly, later, we can test other _all functions, that they don't polute and create _other_ object types on deserialisation (unless expected to). | 16:12 |
| xnox | the above test is valid, with or without full chroot-session deserialisation support. | 16:12 |
| jodh | xnox: sounds good. I'll add that once I've finished test_session_upgrade2()... | 16:14 |
| xnox | jodh: above test, is instead of test_session_upgrade2. | 16:14 |
| xnox | jodh: cause you are trying to test wrong behaviour of session_deserialise_all, by testing the function state_from_string, which is too high up. | 16:15 |
| xnox | and also not abvious. | 16:15 |
| xnox | syntaticly i don't see test_session_upgrade2 any different from test_session_upgrade - it was added to unit-test that: state_from_string returns 0, with json which has sessions in it. | 16:17 |
| jodh | xnox: well, ideally we'd have both as the high-level test is as close as we can realistically get to a full ser/deser test in the unit tests. | 16:17 |
| xnox | (which is valid upgrade scenario we want to catch) | 16:17 |
| xnox | but state_from_string testing is incomplete. | 16:17 |
| jodh | xnox: the only real difference is that the *2 test will run in an environment which has a "valid chroot" in it - that's what found the bug in the first place. | 16:18 |
| xnox | if we had mocking available. I'd be moking out all but one *_all function used inside state_from_string and verify the environment / changes to objects after each one. | 16:18 |
| xnox | and then adding all of their combinations. | 16:18 |
| xnox | jodh: define "valid" =) it's conf_source path actually existed and had job files in it. And the bug is not in state_from_string, but in the session_deserialise_all. | 16:19 |
| jodh | xnox: I know that :) I totally agree we should test the session deser itself, but the high-level test is also valid I think, particularly to allow for when we actually support full deser of chroots. | 16:20 |
| xnox | if each time we find a bug in one of subrutines called by deserialise_*_all, we add a unit test against state_from_string, we are making it harder and harder for ourself to later introduce chroot session deserialisation. | 16:21 |
| jodh | xnox: imagine we refactored the code tomorrow and inadvertantly change the conffile/confsource handling. by having both tests, we're covered. | 16:21 |
| xnox | ok. | 16:21 |
| xnox | jodh: do we have to use pre-created json file though? | 16:23 |
| xnox | jodh: you do nih_file_read, so can do a substituion right there for the temp-path. | 16:25 |
| jodh | xnox: that's what I'm doing atm :) | 16:25 |
| xnox | one session, one conf_source, one job_class - roundtrip should return one session with one conf_source without job_class (and thus no jobs / events / etc), even when that conf_source is on disk. | 16:28 |
| === erkules_ is now known as erkules | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!