/snapcraft

Improve per-step state tracking. #381

Merged
merged 1 commit into from Mar 18, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@kyrofa @coveralls @sergiusens
@kyrofa
Member

kyrofa commented Mar 18, 2016

This PR resolves LP: #1558810 by moving from a single state file to per-step state files. This is necessary for future changes, such as tracking dirty state and per-step cleaning rules.
@coveralls

coveralls commented Mar 18, 2016

Coverage Status

Coverage decreased (-0.1%) to 95.192% when pulling 35b55f3 on kyrofa:feature/1558810/better_state_tracking into 5b3dd6a on ubuntu-core:master.
snapcraft/tests/test_commands_build.py
+ index = common.COMMAND_ORDER.index(expected_step)
+ for step in common.COMMAND_ORDER[:index+1]:
+ self.assertTrue(os.path.exists(os.path.join(state_dir, step)),
+ 'Expected {!r} to be run for {}'.format(step,
@sergiusens

sergiusens Mar 18, 2016

Collaborator

I'm being picky, but can all args for format be on the same next line?

@kyrofa

kyrofa Mar 18, 2016

Member

Ah, I'm sorry @sergiusens you keep asking me to do that and I keep forgetting 😦 . Fixed!

snapcraft/tests/test_commands_pull.py
+ index = common.COMMAND_ORDER.index(expected_step)
+ for step in common.COMMAND_ORDER[:index+1]:
+ self.assertTrue(os.path.exists(os.path.join(state_dir, step)),
+ 'Expected {!r} to be run for {}'.format(step,
@sergiusens

sergiusens Mar 18, 2016

Collaborator

ditto

@kyrofa

kyrofa Mar 18, 2016

Member

Fixed.

snapcraft/tests/test_commands_pull.py
})
return parts
+ def verify_state(self, part_name, state_dir, expected_step):
@sergiusens

sergiusens Mar 18, 2016

Collaborator

This looks like copy paste code, why not put it as part of the general test class or something? @elopio can provide guidance

@kyrofa

kyrofa Mar 18, 2016

Member

Good idea, done.

snapcraft/tests/test_commands_snap.py
- self.state_file = os.path.join(common.get_partsdir(), 'part1', 'state')
+ self.state_dir = os.path.join(common.get_partsdir(), 'part1', 'state')
+
+ def verify_state(self, part_name, expected_step):
@sergiusens

sergiusens Mar 18, 2016

Collaborator

ditto

@kyrofa

kyrofa Mar 18, 2016

Member

Done.

snapcraft/tests/test_commands_stage.py
})
return parts
+ def verify_state(self, part_name, state_dir, expected_step):
@sergiusens

sergiusens Mar 18, 2016

Collaborator

ditto

@kyrofa

kyrofa Mar 18, 2016

Member

Done.

snapcraft/tests/test_commands_strip.py
})
return parts
+ def verify_state(self, part_name, state_dir, expected_step):
@sergiusens

sergiusens Mar 18, 2016

Collaborator

ditto

@kyrofa

kyrofa Mar 18, 2016

Member

Done.

snapcraft/tests/test_commands_upload.py
- self.state_file = os.path.join(common.get_partsdir(), 'part1', 'state')
+ self.statedir = os.path.join(common.get_partsdir(), 'part1', 'state')
+
+ def verify_state(self, expected_step):
@sergiusens

sergiusens Mar 18, 2016

Collaborator

ditto

@kyrofa

kyrofa Mar 18, 2016

Member

Done.

snapcraft/tests/test_pluginhandler.py
@@ -304,6 +343,16 @@ def test_clean_part_already_clean(self, mock_exists, mock_rmtree):
mock_exists.assert_called_once_with(partdir)
self.assertFalse(mock_rmtree.called)
+ def clear_common_directories(self):
@sergiusens

sergiusens Mar 18, 2016

Collaborator

where is this used?

@kyrofa

kyrofa Mar 18, 2016

Member

Ah, good catch! A later PR, it needs to be removed here.

snapcraft/pluginhandler.py
+ if os.path.exists(self._step_state_file(step)):
+ return step
+
+ return None
@sergiusens

sergiusens Mar 18, 2016

Collaborator

How about we create a none step and shove it in COMMAND_ORDER?

snapcraft/pluginhandler.py
+ def is_dirty(self, step):
+ last_step = self.last_step()
+ if last_step:
+ return (common.COMMAND_ORDER.index(step) >
@sergiusens

sergiusens Mar 18, 2016

Collaborator

if we have a none step we can just return this expression, right?

@kyrofa

kyrofa Mar 18, 2016

Member

Yeah that would work, but doing so almost feels a little too clever to me. It'd be difficult to follow.

@sergiusens
Collaborator

sergiusens commented Mar 18, 2016

Looks good; added some comments and also waiting for tests to finish 😄
@coveralls

coveralls commented Mar 18, 2016

Coverage Status

Coverage decreased (-0.03%) to 95.342% when pulling 93fadad on kyrofa:feature/1558810/better_state_tracking into 0c31143 on ubuntu-core:master.
snapcraft/pluginhandler.py
+ for command in common.COMMAND_ORDER[index+1:]:
+ state_file = self._step_state_file(command)
+ if os.path.exists(state_file):
+ os.remove(state_file)
@sergiusens

sergiusens Mar 18, 2016

Collaborator

Now that tests have past and I see coverage has dropped I'm going to be pedantic and say this line isn't tested 😉

https://coveralls.io/builds/5471541/source?filename=snapcraft%2Fpluginhandler.py#L144

And it is true, I don't see a unit test that covers this case.

@kyrofa

kyrofa Mar 18, 2016

Member

Oh that's not pedantic-- done!

@kyrofa
Improve per-step state tracking. 
This is necessary for future changes, such as tracking dirty state
and per-step cleaning rules.

LP: #1558810

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

All checks have passed

4 successful checks
@snappy-m-o
Examples tests — Success 14 tests run, 0 skipped, 0 failed.
Details
@snappy-m-o
autopkgtest — Success No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage decreased (-0.006%) to 95.366%
Details
e7e28f6
@coveralls

coveralls commented Mar 18, 2016

Coverage Status

Coverage decreased (-0.006%) to 95.366% when pulling e7e28f6 on kyrofa:feature/1558810/better_state_tracking into 0c31143 on ubuntu-core:master.
@kyrofa
Member

kyrofa commented Mar 18, 2016

Oh my gosh... coverage to three significant digits...

@kyrofa kyrofa referenced this pull request Mar 18, 2016

Closed

Support cleaning strip step. #382

sergiusens added a commit that referenced this pull request Mar 18, 2016

@sergiusens
Merge pull request #381 from kyrofa/feature/1558810/better_state_trac… 
…king

Improve per-step state tracking.

All checks have passed

2 successful checks
continuous-integration/travis-ci/push — The Travis CI build passed
Details
coverage/coveralls — First build on feature/1537786/per_step_cleaning at 95.366%
Details
3cc0049

@sergiusens sergiusens merged commit 3cc0049 into snapcore:master Mar 18, 2016
4 checks passed

4 checks passed

Examples tests Success 14 tests run, 0 skipped, 0 failed.
Details
autopkgtest Success No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.006%) to 95.366%
Details

@kyrofa kyrofa deleted the kyrofa:feature/1558810/better_state_tracking branch Apr 13, 2016

smoser pushed a commit to smoser/snapcraft that referenced this pull request Sep 14, 2016

@nbirnel @hglkrijger
add python-setuptools to deb build-depends (#381)
f188892

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@sergiusens
Merge pull request #381 from kyrofa/feature/1558810/better_state_trac… 
…king

Improve per-step state tracking.
0b4aefd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment