Skip to content

Fix several issues in CI/tests #5

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

Closed
wants to merge 6 commits into from
Closed

Conversation

mrc0mmand
Copy link
Member

@mrc0mmand mrc0mmand commented Dec 4, 2016

  1. During the review of test results I found out that several test are missing rlGetTestState command at their ends, which results in false-positives (as the default script return code is zero). This should be fixed in commit 75e6128. I also added a simple sanity check, which ensures that all non-library test scripts (runtest.sh) contain the mentioned rlGetTestState. If a script with missing rlGetTestState is encountered, the job is immediately aborted (31dcda2).

  2. After extending the nss test suite, we managed to hit both Travis' limits - job length, which is 50 minutes, and log size, which is 4 MB. Commit ce96c52 implements a simple solution, which allows specifying an environment by an extended bash file glob expression (extglob). I already tested it on the aforementioned nss test suite and it works pretty well.

  3. Last issue concerns the gnutls/softhsm-integration test, which has wrong metadata in its Makefile. This test is unsupported on RHEL/CentOS 6 and has an already fixed bug on RHEL/CentOS 7.2 - in this case we have to simply wait for a docker image for CentOS 7.3. I disabled this test on both RHEL/CentOS 6 and RHEL/CentOS 7 and will create an issue to re-enable it on CentOS 7 once the appropriate docker image is released.

Check if all tests have rlGetTestState command to prevent
false-positives
Centos 6 - unsupported
Centos 7 - there is a bug, which was fixed in RHEL/Centos 7.3
         => re-enable this test when 7.3 docker image is available
@mrc0mmand
Copy link
Member Author

The Travis' fail is caused by the Beakerlib bug mentioned in #7 (rlIsRHEL doesn't work on CentOS). I guess we'll have to wait with merging this PR as well, until it's fixed.

# $2 - glob pattern
function test_name_relevancy() {
# See: http://wiki.bash-hackers.org/syntax/pattern#extended_pattern_language
shopt -s extglob
Copy link
Member

Choose a reason for hiding this comment

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

if we want to preserve the environment, it should first query the state of the setting and then set it, and then restore it, otherwise it will disable it if it is run on a system with it enabled

Copy link
Member Author

Choose a reason for hiding this comment

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

That definitely makes sense, thanks.

@mrc0mmand
Copy link
Member Author

In a few recent runs I noticed a strange fail on Fedora:

:: [  BEGIN   ] :: Running 'rlWait -s 9 18359'
/usr/share/beakerlib/synchronisation.sh: line 89: 18359 Killed                  expect openssl-server.expect openssl s_server -key 1024dsa-server/key.pem -cert 1024dsa-server/cert.pem -CAfile <(cat $(x509Cert ca) ${C_SUBCA[$j]}) -cipher DHE-DSS-AES128-SHA > server.log 2> server.err
:: [   FAIL   ] :: Command 'rlWait -s 9 18359' (Expected 0,143, got 137)

As its occurrence is completely random, it's slightly annoying, because it breaks test results. Re-running the offending job usually helps.

@tomato42
Copy link
Member

tomato42 commented Jan 4, 2017

for that failure, it would be best to tweak the openssl-server.expect script. Which test case was this in?

@mrc0mmand
Copy link
Member Author

Unfortunately, this does happen (or happened at least once) in all test cases which use expect scripts. I spent almost entire weekend trying to find the root cause without any success. For some strange reason it gets stuck after the end of a *-client.expect script. You can find a log with expect debug mode enabled here. Even though it successfully matches the "Server: Generic Web Server" expect section, which should be followed by close and exit 0, it just stops responding instead. close should send an EOF to the spawned process (according to the manpage) which should kill it, but maybe it doesn't...

@tomato42
Copy link
Member

EOF doesn't generally kill the process. It will kill openssl process only if it is actually waiting for input at that very moment. I wonder if making it waif for text that shows up later wouldn't help...

@mrc0mmand
Copy link
Member Author

After another few hours of experimenting and discussing this issue with @ep69 I fixed the problematic client expect scripts. In some cases your idea was enough, in other cases I had to use a sleep - waiting for a following text is kind of hard as expect -re ".+" waits for any following text, which does not mean everything which follows. In these cases we'd have to wait for a specific last line from that particular output or something like that, which may not be possible in all cases. Sleep is a simple and (so far) working solution - hopefully it'll stay this way.

@tomato42
Copy link
Member

looking at the travis failures, looks like we're missing SIGTERM trap in the openssl-server.expect (see: https://stackoverflow.com/questions/23836136/expect-interrupt-program-ctrlc)

@tomato42
Copy link
Member

It still tries to run SHA384 PRF tests with RHEL 6 NSS...

@mrc0mmand
Copy link
Member Author

mrc0mmand commented Jan 30, 2017

It still tries to run SHA384 PRF tests with RHEL 6 NSS...

Yes, this is fixed by #8.

@mrc0mmand mrc0mmand mentioned this pull request Jan 30, 2017
@tomato42
Copy link
Member

tomato42 commented Feb 9, 2017

Yes, this is fixed by #8.

and why it can't be fixed by #5 ?

@mrc0mmand
Copy link
Member Author

mrc0mmand commented Feb 12, 2017

It's a part of the Beakerlib fix, as it adds an alias rlIsRHEL for rlIsCentOS. Without this alias, the condition if ! rlIsRHEL 6; then... for SHA384 PRF is equal to true even on RHEL 6.

@tomato42
Copy link
Member

then lets combine it

@mrc0mmand
Copy link
Member Author

Fixed conflicts and merged all changes into #8.

@mrc0mmand mrc0mmand closed this Feb 20, 2017
@mrc0mmand mrc0mmand deleted the missing-rlGetTestState branch February 25, 2017 21:18
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