Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a unittest for syncing to label revision specifier #173

Merged
merged 8 commits into from
Sep 3, 2020

Conversation

ca-johnson
Copy link
Contributor

Add a unit test to validate syncing to labels works as expected

  1. Follow instructions in test_fixture for how to modify the fixture, creating a label manually
  2. Write assertion about this state of the fixture
  3. Write unit test, checking failures when syncing to non-existent label and success when syncing to test label my-label

Also driveby fix some boilerplate list comprehensions that werent necessary

Relates to #172

@@ -110,7 +110,7 @@ def test_fixture(capsys, server):
}

# Check submitted changes
submitted_changes = [change for change in repo.perforce.run_changes('-s', 'submitted')]
submitted_changes = repo.perforce.run_changes('-s', 'submitted')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't understood why to remove the iteration over the array so far...?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh - the unnecessary list comprehensions driveby, I suppose?

Comment on lines +179 to +184
labelinfo = {
label.get('label'): {key: label.get(key)
for key in ['Revision']
}
for label in labels
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not hissy enough to unpack what's going on here beyond what it says in the comment :(

Copy link
Contributor Author

@ca-johnson ca-johnson Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing that p4 api choses label as the key for what might better be described as a name or identifier - if you imagine label.get('label') was instead label.get('name') it might be more readable..

The result of run_labels() looks like:

labels = [{'label': 'my-label', 'Revision': '@2', 'AnotherKey': 'blah'}, ... more labels]

Then the 'filter' is a dict comprehension, for example:

squared_numbers = {num: num*num for num in range(1, 4)}
                  ^key^ ^value^
# squared_numbers = {1: 1, 2: 4, 3: 9}

In this case we nest one comprehension inside another: one to get each label which then maps to the result of a second comprehension which gets each interesting field (in this case, just 'Revision') for that label

Then labelinfo below is the result of the comprehension

Hope that helps clarify 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does help to clarify, but I'm still not sure I ... comprehend. ;-)

Thanks!

@improbable-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: improbable-mattchurch
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* master:
  Upgrade p4python to 2020.1 (#179)
  ENG-3659 Disable tmp cleanup for P4Python connections (#178)
  Add example workspace cleanup script (#174)
  Revert "Force in case the client has files open (#176)" (#177)
  Force in case the client has files open (#176)
  ENG-3523 Create virtual env once for unique requirements.txt (#167)
…uildkite-plugin into label-unittest

* 'label-unittest' of github.com:improbable-eng/perforce-buildkite-plugin:
@ca-johnson ca-johnson merged commit 35d0069 into master Sep 3, 2020
@ca-johnson ca-johnson deleted the label-unittest branch September 3, 2020 14:14
ca-johnson added a commit that referenced this pull request Sep 3, 2020
* master:
  Add a unittest for syncing to label revision specifier (#173)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants