Ignore reboot skips, SetUpTest and TearDownTest on the subunit report of integration tests. #70

Merged
merged 8 commits into from Nov 11, 2015

Conversation

Projects
None yet
3 participants
Member

elopio commented Nov 10, 2015

When a test requests a reboot, all the remaining tests to run check on their set up if a reboot was requested and skip themselves. Also after the reboot, all the tests that already ran do the same check and skip themselves.
This skips must be ignored on the subunit report of the executed tests.

elopio added some commits Nov 10, 2015

Member

elopio commented Nov 10, 2015

go also announces the start and end of SetUpTest and TearDownTest. I think we should also ignore them, but I'll do that tomorrow.

elopio added some commits Nov 10, 2015

@elopio elopio changed the title from Ignore reboot skips on the subunit report of integration tests. to Ignore reboot skips, SetUpTest and TearDownTest on the subunit report of integration tests. Nov 10, 2015

integration-tests/testutils/report/parser.go
+ } else if matches := skipRegexp.FindStringSubmatch(sdata); len(matches) == 3 {
+ reason := matches[2]
+ // Do not report anything about the set ups skipped because of another test's reboot.
+ duringReboot, _ := regexp.MatchString(
@niemeyer

niemeyer Nov 10, 2015

Contributor

As a rule of thumb, errors should never be ignored. When they are, there should be a comment above the line explaining why it's being ignored. If it's hard to justify, don't ignore it. If it's boring to explain, don't ignore it either, as it's easier to check than to explain.

In this case, for example, a broken regular expression will look silently and exactly the same as an expression that did not match. If you're sure the expression always matches, then create a local helper function such as:

func matchString(pattern string, s string) bool {
        matched, err := regexp.MatchString(pattern, s)
        if err != nil {
                panic(err)
        }
        return matched
}

Same applies to all such cases below.

@elopio

elopio Nov 11, 2015

Member

Ok, I have added this function.

Contributor

niemeyer commented Nov 10, 2015

The logic in this branch is going over my head since I'm not aware of the inner working of subunit, so besides the one issue I could detect it above, I'd suggest picking another review from someone that understand your package. LGTM with that.

elopio added some commits Nov 11, 2015

Member

elopio commented Nov 11, 2015

Thanks for the reviews.
@fgimenez I applied your suggestions, and added the other skips. Ready for another look.

Contributor

fgimenez commented Nov 11, 2015

Ok, 👍

fgimenez added a commit that referenced this pull request Nov 11, 2015

Merge pull request #70 from elopio/ignore_reboot_skips
Ignore reboot skips, SetUpTest and TearDownTest on the subunit report of integration tests.

@fgimenez fgimenez merged commit 7ec70c7 into snapcore:master Nov 11, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 65.735%
Details

@elopio elopio deleted the elopio:ignore_reboot_skips branch Nov 12, 2015

zyga added a commit to zyga/snapd that referenced this pull request Nov 7, 2016

Merge pull request #70 from mwhudson/fix-lintian-warning
fix path in snap-confine.lintian-overrides
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment