Skip to content

Commit

Permalink
Ensure ignored tests are picked up by tox (spotify#2758)
Browse files Browse the repository at this point in the history
* Enable erroneously skipped range tests

Tox has not been running range_test.py due to the file being executable. This commit sets the file permissions to read/write only and remediates the problems that arose when the tests ran again:

* Some tests for RangeMonthly were not accurately capturing the correct logic
* Some tests were not compatible with Python 3 in some map operations
* RangeByMinutesBase performed arithmetics that yield different results on Python 2 and 3, this was changed to be more resilient on both. May have impact on those relying on the erroneous behavior right now.

* Enable erroneously skipped SGE tests

Tox has not been running contrib/sge_test.py due to the file being executable. This commit sets the file permissions to read/write only and remediates the problems that arose when the tests ran again:

* pickle.dump() behaves differently in Python 2 and 3, ensure that it works for both
* Get rid of warnings about unclosed files, handling file lifecycles properly
* Minor docstring syntax
  • Loading branch information
lnsndn authored and Konstantin Gudkov committed Aug 23, 2019
1 parent 6ae3c8b commit b9c53ba
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 14 deletions.
6 changes: 4 additions & 2 deletions luigi/contrib/sge.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,11 @@ def _dump(self, out_dir=''):
d = pickle.dumps(self)
module_name = os.path.basename(sys.argv[0]).rsplit('.', 1)[0]
d = d.replace('(c__main__', "(c" + module_name)
open(self.job_file, "w").write(d)
with open(self.job_file, "w") as f:
f.write(d)
else:
pickle.dump(self, open(self.job_file, "w"))
with open(self.job_file, "wb") as f:
pickle.dump(self, f)

def _run_job(self):

Expand Down
2 changes: 1 addition & 1 deletion luigi/tools/range.py
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ def finite_datetimes(self, finite_start, finite_stop):
# Validate that the minutes_interval can divide 60 and it is greater than 0 and lesser than 60
if not (0 < self.minutes_interval < 60):
raise ParameterException('minutes-interval must be within 0..60')
if (60 / self.minutes_interval) * self.minutes_interval != 60:
if 60 % self.minutes_interval != 0:
raise ParameterException('minutes-interval does not evenly divide 60')
# start of a complete interval, e.g. 20:13 and the interval is 5 -> 20:10
start_minute = int(finite_start.minute/self.minutes_interval)*self.minutes_interval
Expand Down
8 changes: 4 additions & 4 deletions test/contrib/sge_test.py
100755 → 100644
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def on_sge_master():
class TestSGEWrappers(unittest.TestCase):

def test_track_job(self):
'''`track_job` returns the state using qstat'''
"""`track_job` returns the state using qstat"""
self.assertEqual(_parse_qstat_state(QSTAT_OUTPUT, 1), 'r')
self.assertEqual(_parse_qstat_state(QSTAT_OUTPUT, 2), 'qw')
self.assertEqual(_parse_qstat_state(QSTAT_OUTPUT, 3), 't')
Expand All @@ -63,7 +63,7 @@ def test_track_job(self):

class TestJobTask(SGEJobTask):

'''Simple SGE job: write a test file to NSF shared drive and waits a minute'''
"""Simple SGE job: write a test file to NSF shared drive and waits a minute"""

i = luigi.Parameter()

Expand All @@ -79,7 +79,7 @@ def output(self):
@attr('contrib')
class TestSGEJob(unittest.TestCase):

'''Test from SGE master node'''
"""Test from SGE master node"""

def test_run_job(self):
if on_sge_master():
Expand All @@ -94,7 +94,7 @@ def test_run_job_with_dump(self, mock_check_output):
'Your job 12345 ("test_job") has been submitted',
''
]
task = TestJobTask(i=1, n_cpu=1, shared_tmp_dir='/tmp')
task = TestJobTask(i="1", n_cpu=1, shared_tmp_dir='/tmp')
luigi.build([task], local_scheduler=True)
self.assertEqual(mock_check_output.call_count, 2)

Expand Down
15 changes: 8 additions & 7 deletions test/range_test.py
100755 → 100644
Original file line number Diff line number Diff line change
Expand Up @@ -1100,7 +1100,7 @@ class MyTask(luigi.Task):
def complete(self):
return False

range_task = RangeMonthly(now=datetime_to_epoch(datetime.datetime(2015, 12, 2)),
range_task = RangeMonthly(now=datetime_to_epoch(datetime.datetime(2016, 1, 1)),
of=MyTask,
start=datetime.date(2015, 12, 1),
stop=datetime.date(2016, 1, 1))
Expand All @@ -1125,7 +1125,7 @@ def run(self):
self.comp = True
MyTask.secret = 'yay'

now = str(int(datetime_to_epoch(datetime.datetime(2015, 12, 2))))
now = str(int(datetime_to_epoch(datetime.datetime(2016, 1, 1))))
self.run_locally_split('RangeMonthly --of wohoo.MyTask --now {now} --start 2015-12 --stop 2016-01'.format(now=now))
self.assertEqual(MyTask(month_param=datetime.date(1934, 12, 1)).secret, 'yay')

Expand All @@ -1137,7 +1137,7 @@ class MyTask(luigi.Task):
def complete(self):
return False

range_task = RangeMonthly(now=datetime_to_epoch(datetime.datetime(2015, 12, 2)),
range_task = RangeMonthly(now=datetime_to_epoch(datetime.datetime(2016, 1, 1)),
of=MyTask,
start=datetime.date(2015, 12, 1),
stop=datetime.date(2016, 1, 1),
Expand All @@ -1153,7 +1153,7 @@ class MyTask(luigi.Task):
def output(self):
return MockTarget(self.month_param.strftime('/n2000y01a05n/%Y_%m-aww/21mm%Hdara21/ooo'))

range_task = RangeMonthly(now=datetime_to_epoch(datetime.datetime(2015, 12, 2)),
range_task = RangeMonthly(now=datetime_to_epoch(datetime.datetime(2016, 1, 1)),
of=MyTask,
start=datetime.date(2015, 12, 1),
stop=datetime.date(2016, 1, 1),
Expand Down Expand Up @@ -1197,7 +1197,7 @@ def run(self):
self.comp = True
MyTask.state = (self.arbitrary_param, self.arbitrary_integer_param)

now = str(int(datetime_to_epoch(datetime.datetime(2015, 12, 2))))
now = str(int(datetime_to_epoch(datetime.datetime(2016, 1, 1))))
self.run_locally(['RangeMonthly', '--of', 'wohoo.MyTask',
'--of-params', '{"arbitrary_param":"bar","arbitrary_integer_param":5}',
'--now', '{0}'.format(now), '--start', '2015-12', '--stop', '2016-01'])
Expand Down Expand Up @@ -1444,7 +1444,7 @@ class BulkCompleteByMinutesTask(luigi.Task):

@classmethod
def bulk_complete(cls, parameter_tuples):
return parameter_tuples[:-2]
return list(parameter_tuples)[:-2]

def output(self):
raise RuntimeError("Shouldn't get called while resolving deps via bulk_complete")
Expand All @@ -1471,9 +1471,10 @@ class BulkCompleteByMinutesTask(luigi.Task):

@classmethod
def bulk_complete(cls, parameter_tuples):
ptuples = list(parameter_tuples)
for t in map(cls, parameter_tuples):
assert t.arbitrary_argument
return parameter_tuples[:-2]
return ptuples[:-2]

def output(self):
raise RuntimeError("Shouldn't get called while resolving deps via bulk_complete")
Expand Down

0 comments on commit b9c53ba

Please sign in to comment.