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

Assert more about the test server fixture #140

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Conversation

ca-johnson
Copy link
Contributor

Improve code-as-documentation on what the server fixture contains

In lieu of work on issue #111

Comment on lines +106 to +107
"//depot/file.txt": "Hello World\n",
"//stream-depot/main/file.txt": "Hello Stream World\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content of files

Comment on lines +119 to +135
assert submitted_changeinfo == {
'1' :{
'action': ['add'],
'depotFile': ['//depot/file.txt'],
'desc': 'Initial Commit'
},
'2' :{
'action': ['add'],
'depotFile': ['//stream-depot/main/file.txt'],
'desc': 'Initial Commit to Stream\n'
},
'6' :{
'action': ['edit'],
'depotFile': ['//depot/file.txt'],
'desc': 'modify //depot/file.txt\n'
},
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If tests fail, it shows you a rich diff of exactly which bits were different

'action': ['edit'],
'depotFile': ['//depot/file.txt'],
'desc': 'Modify file in shelved change\n',
# Change content from 'Hello World\n' to 'Goodbye World\n'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hopefully the comment suffices here
If there are more complex shelved changes created we could look at fetching the content of the shelved change and including it here. Omitted for the sake of simplicity for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually on second though this might be really useful to include the content, as it means we can put this static info in a global var for the unit tests and refer to it from throughout, avoiding potential mistakes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe in a future PR...

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be up for making that, it's been a while since I python'd.

@ca-johnson
Copy link
Contributor Author

@commanderofthegrey Could you take a look at this? Thanks

Copy link
Contributor

@petemounce petemounce left a comment

Choose a reason for hiding this comment

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

LGTM; are other EV reviewing too, to learn?

python/test_perforce.py Outdated Show resolved Hide resolved
}

# Check submitted changes
submitted_changes = [change for change in 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.

Is there a long-form flag instead of -s, for readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately not, it means status


# Check shelved changes
shelved_changes = [change for change in repo.perforce.run_changes('-s', 'pending')]
shelved_changeinfo = {change["change"]: repo.perforce.run_describe('-S', change["change"])[0] for change in shelved_changes}
Copy link
Contributor

Choose a reason for hiding this comment

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

Long form for -S?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately not, it means get info for shelved files on this change
hopefully this is implied from the var it is being assigned to

python/test_perforce.py Outdated Show resolved Hide resolved
@filipVisko
Copy link

I'm not sure I understand enough what is going on... :/

@ca-johnson
Copy link
Contributor Author

We have a zip of a perforce server which is used to run unit tests against and this asserts around some state it has, otherwise its a bit of a 'black box' as to what is on the server

@ca-johnson
Copy link
Contributor Author

@ca-johnson ca-johnson merged commit e8e61c5 into master Jan 28, 2020
@ca-johnson ca-johnson deleted the fixture-asserts-detail branch January 28, 2020 19:48
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.

None yet

3 participants