#general
- w
witty-crayon-22786
08/31/2015, 12:03 AMright - b
bored-art-40741
08/31/2015, 12:04 AMYeah, I agree this is testing the error case properly. But if I could nit: this is a sufficiently subtle (but also important) case that I'd suggest putting it into its own test with a comment or docstring indicating its purpose - b
bored-art-40741
08/31/2015, 12:04 AMOr even just a comment/docstring on the existing test indicating that it's dual purpose - w
witty-crayon-22786
08/31/2015, 12:04 AMi changed the comment on the whitelist test to indicate that it tests both success and failure - w
witty-crayon-22786
08/31/2015, 12:05 AM"""Ensure that a project missing dependencies fails if it is not whitelisted.""" - b
bored-art-40741
08/31/2015, 12:05 AMYeah, I see that. Maybe this is from my general unfamiliarity with our integration test base classes, but having read that a few times it wasn't apparent to me that this was testing multiple iterations with caching - b
bored-art-40741
08/31/2015, 12:06 AManyway, it was a nit, not a proper issue - b
bored-art-40741
08/31/2015, 12:06 AMYou have my ship, and thanks for taking a look on short notice - w
witty-crayon-22786
08/31/2015, 12:06 AMnp - w
witty-crayon-22786
08/31/2015, 12:08 AMer... apparently a context manager won't run if its result isn't used. sec. - w
witty-crayon-22786
08/31/2015, 12:12 AMis there a shorthand to just consume the context manager? - w
witty-crayon-22786
08/31/2015, 12:13 AM.next ? - e
enough-analyst-54434
08/31/2015, 12:13 AMnot really: `context = contexmanager(); context.gen.next(); context.gen.next() - e
enough-analyst-54434
08/31/2015, 12:13 AMcatch the raise on #2 - e
enough-analyst-54434
08/31/2015, 12:13 AMmuch much easier to with pass - e
enough-analyst-54434
08/31/2015, 12:13 AMand comment the pass - w
witty-crayon-22786
08/31/2015, 12:13 AMmm. so just having a block is probably cleaner. - w
witty-crayon-22786
08/31/2015, 12:13 AMyea. - e
enough-analyst-54434
08/31/2015, 12:13 AMyes - w
witty-crayon-22786
08/31/2015, 12:13 AMgoing to fix two other tests while i'm in here =x - e
enough-analyst-54434
08/31/2015, 12:14 AMYou can alternatively write your contextmanager thing with support for both methods of call - w
witty-crayon-22786
08/31/2015, 12:26 AMthe old "never trust a test you haven't seen fail" adage applies - w
witty-crayon-22786
08/31/2015, 12:37 AM@bored-art-40741: ok, those tests now properly run (!) and confirm stability for global. - b
bored-art-40741
08/31/2015, 12:37 AMyay - w
witty-crayon-22786
08/31/2015, 12:37 AMbut it looks like missing direct deps are not detected for isolated. i'd like to follow up on that now - w
witty-crayon-22786
08/31/2015, 12:37 AMbut get this test in first, if that's ok with you - w
witty-crayon-22786
08/31/2015, 12:37 AM@bored-art-40741: ^ - b
bored-art-40741
08/31/2015, 12:38 AMStrictly better than now, and alleviates my concerns - b
bored-art-40741
08/31/2015, 12:38 AM+1 - w
witty-crayon-22786
08/31/2015, 12:38 AMk