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 unit tests #54

Merged
merged 11 commits into from
Jul 21, 2023
Merged

Add unit tests #54

merged 11 commits into from
Jul 21, 2023

Conversation

carlcsaposs-canonical
Copy link
Contributor

@carlcsaposs-canonical
Copy link
Contributor Author

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #54 (4e2957d) into main (e1d39d4) will increase coverage by 23.21%.
The diff coverage is 73.34%.

@@             Coverage Diff             @@
##             main      #54       +/-   ##
===========================================
+ Coverage   50.23%   73.45%   +23.21%     
===========================================
  Files           7       14        +7     
  Lines         428      953      +525     
  Branches       57      110       +53     
===========================================
+ Hits          215      700      +485     
- Misses        200      223       +23     
- Partials       13       30       +17     
Impacted Files Coverage Δ
...elations/deprecated_shared_db_database_provides.py 37.57% <37.57%> (ø)
src/workload.py 57.62% <57.62%> (ø)
src/snap.py 64.28% <64.28%> (ø)
src/lifecycle.py 64.91% <64.91%> (ø)
src/abstract_charm.py 82.47% <82.47%> (ø)
src/mysql_shell.py 83.75% <83.75%> (ø)
src/relations/database_providers_wrapper.py 84.61% <84.61%> (ø)
src/machine_charm.py 86.11% <86.11%> (ø)
src/socket_workload.py 92.85% <92.85%> (ø)
src/container.py 94.54% <94.54%> (ø)
... and 4 more

Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

Well done!

assert state.relations[-1].local_app_data == {}
complete_provides_s.pop()
for index, provides in enumerate(complete_provides_s, 1):
assert state.relations[index].local_app_data == {

Choose a reason for hiding this comment

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

take a look at State.get_relations, maybe it can be handy for filtering out what relations you want to look at.

I spent some time thinking about how it'd be handy to expose the relations, but I wasn't sure.

would a get_relation_by_id be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

would a get_relation_by_id be better?

Yes I think so—so that we can use the input scenario.Relation to access the output scenario.Relation

If you look at this example:

for index, provides in enumerate(complete_provides_s, 1):
local_app_data = state.relations[index].local_app_data
assert len(local_app_data.pop("password")) > 0
assert local_app_data == {
"database": provides.remote_app_data["database"],
"endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysql.sock",
"read-only-endpoints": "file:///var/snap/charmed-mysql/common/run/mysqlrouter/mysqlro.sock",
"username": f'{complete_requires.remote_app_data["username"]}-{provides.relation_id}',
}
for index, provides in enumerate(incomplete_provides_s, 1 + len(complete_provides_s)):
assert state.relations[index].local_app_data == {}

complete_provides_s and incomplete_provides_s relations have the same endpoint, but we need to make different assertions on them

Choose a reason for hiding this comment

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

yeah precisely. I had this issue: canonical/ops-scenario#20 to track ideas and proposals around this use case

Choose a reason for hiding this comment

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

something like obj_out = state_out.get_me_corresponding_object(obj_in)

carlcsaposs-canonical added a commit that referenced this pull request Jul 19, 2023
CI should not fail if code coverage drops

Except for TLS and the legacy shared-db relation (which I skipped in #54), adding additional unit testing would have, imo, little to negative value since it would require extensive mocking and would essentially only be testing if the internal APIs of the charm have changed (which they should, as the charm evolves and is refactored)

Therefore, I don't think it's useful to have a failed CI run if code coverage drops
CI should not fail if code coverage drops

Except for TLS and the legacy shared-db relation (which I skipped in
#54), adding additional unit testing would have, imo, little to negative
value since it would require extensive mocking and would essentially
only be testing if the internal APIs of the charm have changed (which
they should, as the charm evolves and is refactored)

Therefore, I don't think it's useful to have a failed CI run if code
coverage drops
@carlcsaposs-canonical carlcsaposs-canonical merged commit b043242 into main Jul 21, 2023
10 of 11 checks passed
@carlcsaposs-canonical carlcsaposs-canonical deleted the scenario branch July 21, 2023 14:22
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