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 acceptance testing using packages built during testing #1340

Merged
merged 6 commits into from
Aug 19, 2021
Merged

Conversation

treydock
Copy link
Contributor

This fixes #1333. I also tried to make everything work so that when we add Ubuntu/Debian packages, just update the conditions to adapt logic based on host inventory information for those systems.

The previous E2E tests were copied to spec/support as shared examples except the pre pun hooks because couldn't find a clean way to abstract out the extra logic differences between acceptances added in this PR and previous E2E tests.

In case it's not obvious, there are 2 things that ensure the ondemand-release repo file does NOT install the main ondemand and ondemand-gems RPMs. First I set the local repo priority to be higher than main ondemand-web repo (lower priority number is higher priority in yum) and second I modify ondemand-web repo to exclude the RPMs being tested. The actual ondemand-release repo file was needed to install ondemand-dex and the ondemand runtime RPMs for SCL as well as passenger and NGINX.

Some of the logic is extra like only adding ood user if not already present. This is useful because you can do bundle exec rake test:acceptance BEAKER_set=el7 BEAKER_destroy=no and the container is reused and not destroyed after tests so can be inspected and so various setup steps need to be idempotent. Also makes testing minor changes faster and easier as don't have to wait for all the bootstrapping logic to re-run if just changing one test.

There is 1 bug fix and 1 feature I am using with the beaker-docker code. The bug fix isn't necessary with older versions of beaker-docker gem but would still need the feature in order to expose ports besides SSH. The beaker logic only exposes port for SSH so had to add feature to expose extra ports. Normally when I write tests to connect to web services for testing I use curl from within the container, but to use the E2E logic required exposing those ports like we do for E2E tests.

Also added -C flag to RPM building so that you can build el7 and then el8 and the build script won't cleanup the dist/* locations of previous builds.

@treydock
Copy link
Contributor Author

For some reason locally running tests the EL7 tests work just fine but EL8 I get this when I log into OnDemand:

Screen Shot 2021-08-13 at 1 59 41 PM

The RPM should be handling sys app initialization as it's in RPM spec but I see this:

[root@centos-8 /]# ls -la /var/lib/ondemand-nginx/config/apps/sys/
total 8
drwxr-xr-x 2 root root 4096 Aug 13 17:35 .
drwxr-xr-x 5 root root 4096 Aug 13 17:57 ..
[root@centos-8 /]# 

@treydock
Copy link
Contributor Author

And this is why these tests will be so useful, running the tests locally I see this:

  Running scriptlet: ondemand-2.0.15-1.el8.x86_64                                                                       1/2 
/var/tmp/rpm-tmp.5rxZOf: line 2: syntax error near unexpected token `fi'
/var/tmp/rpm-tmp.5rxZOf: line 2: `fi'
warning: %post(ondemand-2.0.15-1.el8.x86_64) scriptlet failed, exit status 2

Error in POSTIN scriptlet in rpm package ondemand

Not sure if that's why some files are missing from RPM but it's a start.

@treydock
Copy link
Contributor Author

Well I know why this broke things. The first thing in %post was breaking due to bad syntax that only happens for non-SCL systems ie EL8 and after that failure is where the RPM spec was touching the files needed to initialize.

@johrstrom
Copy link
Contributor

I don't think I can look into this in depth today, but I think I'd prefer to break it up. What I mean is leave e2e tests as they are for the moment. They will go into this similar to what you have, it's just easier for me to see it one thing at a time. Plus I think we'd want to start to build the e2e images with the packages we just built anyhow, so that's another chance we can sort of iterate towards.

I also don't care for the word 'acceptance'. Can we call it 'package'?

Put e2e tests back to way they were before this PR
Simplify GH action for testing packages
@treydock
Copy link
Contributor Author

treydock commented Aug 13, 2021

So I can imagine the package tests replacing the E2E tests because the E2E tests are going to be testing the same things work just building from source rather than building packages. The way these tests work is there is only a base OS image involved, once that base OS image is made, it's treated like a VM or provisioned host and the OnDemand environment is installed into it just like we document on website. There is no container building with these new tests other than to bootstrap a docker OS environment and that's handled by beaker-docker Gem.

I made the changes to replace acceptance with package and also put e2e tests back to original state but made a copy of them into spec/support so the new packaging tests use those shared examples which are pretty much just copies of E2E tests.

@treydock
Copy link
Contributor Author

I also simplified the GH Action to not require uploading artifacts from one workflow and downloading them into another, just added another step to existing package building workflow that will test the built packages, so no upload/download involved with latest commit.

@treydock
Copy link
Contributor Author

@johrstrom Let me know if you've had a chance to review this. This does include one bug fix for EL8 RPMs that was discovered thanks to the new package testing. Kind of surprised no one reported this issue sooner, maybe they just assumed it was normal to have to always initialize sys apps upon first launch.

@johrstrom
Copy link
Contributor

I haven't gotten a chance to look at it in detail, but I'll carve out time tonight.

Do you use a linter? I'm trying to start to enforce most things things in our .rubocop.yml at the top level directory. I know for sure
there's a Layout/HeredocIndentation and Style/ConditionalAssignment here. Don't worry about Style/WordArray - we need to enforce arrays of strings.

@treydock
Copy link
Contributor Author

How are you running rubocop? I am used to using it with Rake tasks so you can do something like rake rubocop and then to fix can try rake rubocop:auto_correct.

@johrstrom
Copy link
Contributor

I use vscode with solargraph so I see stuff like this. I have the extension for the OnDemand vscode too if you want it (it's not in the marketplace I had to build it myself).

image

Some stuff we either can't avoid or just don't need to worry about like function length, but I do want to start having some consistency across the code base. I haven't tried rake rubocop:auto_correct - but happy to make a new ticket(s) to add it to places.

@treydock
Copy link
Contributor Author

I think a top-level Rake task makes sense and update lib/.rubocop.yml or some such file to match what we want, because some things you mentioned not caring about are not ignored by that. I ran this to get latest commit:

rubocop -c lib/.rubocop.yml spec/support/*.rb -a
rubocop -c lib/.rubocop.yml spec/support/*.rb -A
rubocop -c lib/.rubocop.yml spec/package/*.rb -a
rubocop -c lib/.rubocop.yml spec/package/*.rb -A

@johrstrom
Copy link
Contributor

That's legit.

Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

OK this is fine. I didn't realize that you'd still have all (or most) the e2e tests still in this PR but that's fine. We can refactor all that in another ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI tests to verify RPMs
3 participants