Skip to content
This repository was archived by the owner on Sep 23, 2024. It is now read-only.

Remove baseline rechecking #277

Merged
merged 1 commit into from
Jul 12, 2018

Conversation

spbnick
Copy link
Contributor

@spbnick spbnick commented Jul 12, 2018

Baseline rechecking is confusing and its failures are difficult to
detect in sktm. Skt shouldn't really be doing it, since it wasn't skt
which decided to check baseline in the first place.

Baseline rechecking is confusing and its failures are difficult to
detect in sktm. Skt shouldn't really be doing it, since it wasn't skt
which decided to check baseline in the first place.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1005

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 76.637%

Totals Coverage Status
Change from base Build 1003: 0.7%
Covered Lines: 1112
Relevant Lines: 1416

💛 - Coveralls

@veruu
Copy link
Contributor

veruu commented Jul 12, 2018

While I agree with the sentiment of the patch, I want to be sure this won't cause us report false failures.

IIRC we are passing the latest stable baseline from sktm, so the baseline shouldn't fail here. Unless it's a host-specific failure which occurs with baseline as well (and these chosen hosts weren't tested when the baseline was marked as stable), which is what this chunk of the code is trying to rule out.

What are the chances that we randomly get two hosts which fail and need to verify if it's the hosts or the patches that are broken (or even tests, which should be caught here too)?

@spbnick
Copy link
Contributor Author

spbnick commented Jul 12, 2018

Let me just say this first: there are no records of baseline failures in our database at all. I.e. they were never detected. I don't think I've ever seen a Beaker job for a baseline check either.

Now, the code being removed is making an assumption that the baseline build is going to be present at the same location the build it is testing is published at, which is not skt's business to know, at all. The proper (maintainable and usable) way of implementing this would be to explicitly supply skt with the baseline build (in Beaker runner parameters), and to let skt report ERROR (not PASS) if it fails to find a host where baseline works. Something along that line, anyway, and not what's being done right now.

@veruu
Copy link
Contributor

veruu commented Jul 12, 2018

I don't think the DB entries are relevant. What the code does is "everything failed, let me see if it's bug with the series or not". If this chunk of code never ran, then it answers my question about the probability and I'm fine with the removal :)

Agreed with the rest of your comment. That said, should that change be implemented as well?

@spbnick
Copy link
Contributor Author

spbnick commented Jul 12, 2018

We would be better off "running light" for the report refactoring and adding baseline checking afterwards, lest it slows us down.

@veruu
Copy link
Contributor

veruu commented Jul 12, 2018

Agreed. Can you submit an issue that would describe the problem and possible solution so we can track it and not forget about it, and I'll ack this patch?

@spbnick
Copy link
Contributor Author

spbnick commented Jul 12, 2018

@veruu, would cki-project/meta#7 do?

@veruu
Copy link
Contributor

veruu commented Jul 12, 2018

Yup! Thank you!

@spbnick spbnick merged commit f108292 into cki-project:master Jul 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants