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

Pass arguments BaseCharmTest.setUpClass #257

Merged
merged 1 commit into from May 5, 2020
Merged

Pass arguments BaseCharmTest.setUpClass #257

merged 1 commit into from May 5, 2020

Conversation

dshcherb
Copy link
Contributor

@dshcherb dshcherb commented May 4, 2020

Not doing so triggers an incorrect behavior leading to functional test
failures as the application name is not set correctly.

#256
https://review.opendev.org/#/c/712980/1

Not doing so triggers an incorrect behavior leading to functional test
failures as the application name is not set correctly.

#256
https://review.opendev.org/#/c/712980/1
@codecov-io
Copy link

Codecov Report

Merging #257 into master will increase coverage by 0.44%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   20.35%   20.80%   +0.44%     
==========================================
  Files         131      131              
  Lines        7250     7250              
==========================================
+ Hits         1476     1508      +32     
+ Misses       5774     5742      -32     
Impacted Files Coverage Δ
zaza/openstack/charm_tests/test_utils.py 23.35% <100.00%> (+23.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b821c4c...740bc20. Read the comment docs.

@ajkavanagh
Copy link
Contributor

This is a great catch. It's also a breaking change? It would be good to see some test runs against a few charms to check that there's no unexpected behaviour in other tests that may cause us trouble -- we're 3 days from freeze! Please could we test against, say (in a bastion):

  • aodh
  • cinder
  • nova-compute

and verify there's no weird behaviour.

Note. I may be being over-cautious here, in which case, please feel free to comment!

openstack-gerrit pushed a commit to openstack/charm-designate-bind that referenced this pull request May 5, 2020
Zaza tests are already implemented

Change-Id: I59b52f3736029e134856a2bee69dd2b5037e9d52
Closes-Bug: #1828424
Func-Test-PR: openstack-charmers/zaza-openstack-tests#257
openstack-gerrit pushed a commit to openstack/openstack that referenced this pull request May 5, 2020
* Update charm-designate-bind from branch 'master'
  - Remove Amulet leftovers
    
    Zaza tests are already implemented
    
    Change-Id: I59b52f3736029e134856a2bee69dd2b5037e9d52
    Closes-Bug: #1828424
    Func-Test-PR: openstack-charmers/zaza-openstack-tests#257
@dshcherb
Copy link
Contributor Author

dshcherb commented May 5, 2020

@ajkavanagh The change that introduced the issue was done quite recently: 9cb7876e

So I think it's not a breaking change as far as the older tests go but the ones merged between then and now might be affected.

To find out whether any of them are I did the following:

# None of the lines are prefixed with `+` which means that the lines below are just the lines around new changes that were there before
git log -p 9cb7876e..master | grep -P '\.setUpClass\((\w|\d)+\)'
         super(NeutronOpenvSwitchTest, cls).setUpClass(cls)
         super(GenericPolicydTest, cls).setUpClass(application_name)
         super(BasePolicydSpecialization, cls).setUpClass(application_name)
         super(NeutronGatewayTest, cls).setUpClass(cls)

# Gives more results just for sanity checking and catching multi-line cases
git log -p 9cb7876e..master | grep -P 'setUpClass\('

There is a lesser possibility that some of those test cases began to pass after 9cb7876 was merged.

This change managed to fix it for https://review.opendev.org/#/c/712980/

@ajkavanagh
Copy link
Contributor

@dshcherb okay, I think we are probably pretty safe then. Let's get this merged :)

@ajkavanagh ajkavanagh merged commit 08177ef into openstack-charmers:master May 5, 2020
lolwww pushed a commit to lolwww/charm-designate-bind that referenced this pull request Mar 9, 2023
Zaza tests are already implemented

Change-Id: I59b52f3736029e134856a2bee69dd2b5037e9d52
Closes-Bug: #1828424
Func-Test-PR: openstack-charmers/zaza-openstack-tests#257
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.

OpenStackBaseTest doesn't pass setUpClass args to setUpClass of BaseCharmTest
4 participants