Skip to content

Conversation

@opalmer
Copy link
Member

@opalmer opalmer commented Oct 26, 2015

This adds additional tests for the JobType class and increases coverage without modifying the JobType class itself.

@opalmer opalmer self-assigned this Oct 26, 2015
@opalmer opalmer added this to the 0.8.7 milestone Oct 26, 2015
This is a potentially breaking change.  Before this we would
simply log a warning if the configuration defined jobtype_default_environment
and it was not a dictionary.  From the perspective of job type
implementor this would have been a silent failure and resulted in
potentially unexpected behavior within a running job.
@opalmer
Copy link
Member Author

opalmer commented Oct 27, 2015

@guidow, PTAL at 23cb112 when you get a chance. This is one potentially breaking change I think this CL will be introducing. The current implementation IMHO is currently broken because it ignores broken configuration entries (which in turn could break a job/task).

I've moved this into #345.

@opalmer opalmer changed the title [WIP] JobType Coverage General JobType Coverage Nov 1, 2015
@opalmer
Copy link
Member Author

opalmer commented Nov 1, 2015

@guidow, PTAL. There's still some places I can add coverage, and plan to, but I figured this was probably complete enough that it's ready for review.

@guidow
Copy link
Contributor

guidow commented Jan 28, 2016

There is a bit of a lull in my current project, so I'm taking some time to look at these PRs. Sorry again for keeping you waiting this long.

This one fails a test on my machine:

[FAIL]
Traceback (most recent call last):
File "/home/guido/git/pyfarm/pyfarm-agent/tests/test_jobtypes/test_core_jobtype.py", line 567, in test_uses_provided_create_time
    filename
File "/var/tmp/virtualenv/pyfarm-agent-python2/lib/python2.7/site-packages/twisted/trial/_synctest.py", line 437, in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
File "/usr/lib64/python2.7/unittest/case.py", line 515, in assertEqual
    assertion_func(first, second, msg=msg)
File "/usr/lib64/python2.7/unittest/case.py", line 508, in _baseAssertEqual
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: '2016-1-28_14-47-56_1_0c6d2ebffb0c402aadab173b633a79bf' != '2016-01-28_14-47-56_1_0c6d2ebffb0c402aadab173b633a79bf'

tests.test_jobtypes.test_core_jobtype.TestJobTypeGetCSVLogPath.test_uses_provided_create_time

Apparently, a naming function for the CVS-log path uses a leading zero for the month in one place and no leading zero in another.

This would also explain why the test passed with no apparent problems back in November...

@guidow
Copy link
Contributor

guidow commented Jan 28, 2016

Other than that, this looks okay.

@opalmer
Copy link
Member Author

opalmer commented Jan 29, 2016

No worries, production can be like that so I understand. Looks like #352 should fix the test above so we can probably merge that PR and this one once #353 merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants