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 test to ensure we have a dracut configuration that contains all… #278

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

rjschwei
Copy link
Collaborator

… the

drivers necessary to support instance type switching between nitro and
xen based instances

Copy link
Collaborator

@smarlowucf smarlowucf left a comment

Choose a reason for hiding this comment

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

This would be better as a paramterized test. That way each driver is tested individually and it makes failure explicit. E.g. a paramterized test would look like test_sles_dracut_conf[ena]. See https://github.com/SUSE-Enceladus/img-proof/blob/master/usr/share/lib/img_proof/tests/SLES/EC2/test_sles_ec2_services.py.

@rjschwei
Copy link
Collaborator Author

This would be better as a paramterized test. That way each driver is tested individually and it makes failure explicit. E.g. a paramterized test would look like test_sles_dracut_conf[ena]. See https://github.com/SUSE-Enceladus/img-proof/blob/master/usr/share/lib/img_proof/tests/SLES/EC2/test_sles_ec2_services.py.

But that also implies we are making 7 connections to the host to read the same file 7 times unless we create a global cache of the content. Not sure the global cache is an elegant solution. But reading the same file from the host multiple times is not a good idea.

@smarlowucf
Copy link
Collaborator

smarlowucf commented Nov 16, 2020

Given pytest parses the test at once I believe it's only one connection. I don't really see the concern about opening a file seven times. To me avoiding that without seeing a noticeable performance issue would be premature optimization.

It will also become a big headache if the test fails and is not parameterized because even with the API right now you only see test pass/fail. Which means you'd have no idea which driver is missing.

@rjschwei
Copy link
Collaborator Author

Well it would not pre-mature optimization if in fact a network connection is established to read the same file 7 times over. Anyway, I do not have the spare time to dig into what pytest actually does, I don't like the idea of a global cache and do not like the loop. SO I guess we'lll just not test for this. We'd have to establish what pytest actually does in order to move forward.

@rjschwei rjschwei closed this Nov 16, 2020
@rjschwei rjschwei deleted the ec2tswitch branch November 16, 2020 19:30
@smarlowucf smarlowucf restored the ec2tswitch branch November 16, 2020 19:49
@smarlowucf smarlowucf reopened this Nov 16, 2020
@smarlowucf
Copy link
Collaborator

Re-opened, I don't see the point of ignoring the test.

'xen-blkfront', 'xen-netfront')
dracut_conf = host.file('/etc/dracut.conf.d/07-aws-type-switch.conf')
for driver in needed_drivers:
assert dracut_conf.contains(drivers)
Copy link
Collaborator

Choose a reason for hiding this comment

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

/s/drivers/driver

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Typo fixed. But again I am happy to pull this if you think the loop is not OK, the close was just intended to not leave this dangling for investigation into pytest behavior with respect to network connection opening. Alternative to parameterize and avoid the investigation into pytest


dracut_conf = ''

@pytest....
def test_sles_dracut_conf(host,driver):
    global dracut_conf
    if not dracut_conf:
         dracut_conf = host.file('/etc/dracut.conf.d/07-aws-type-switch.conf')
    assert dracut_conf.contains(driver)

should work, but that has the global which is not particularly pretty either

… the

  drivers necessary to support instance type switching between nitro and
  xen based instances
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.

2 participants