Skip to content
This repository has been archived by the owner on Oct 4, 2019. It is now read-only.

"Fix"/schroedinger tests #543

Merged
merged 31 commits into from
May 7, 2018
Merged

"Fix"/schroedinger tests #543

merged 31 commits into from
May 7, 2018

Conversation

whilei
Copy link
Contributor

@whilei whilei commented Mar 23, 2018

  • consider what to do with schroedingers_cat.go -- move to janus? relocate somewhere else? leave as-is? moved to https://github.com/etcdevteam/go-schroedinger, and use go get to install command on CIs
  • possibly rename "schroedinger" things to be something a little more "professional" :bowtie: ...

Schrödinger coined the term Verschränkung (entanglement) in the course of developing the thought experiment. https://en.wikipedia.org/wiki/Schr%C3%B6dinger's_cat

  • include more nondeterministic tests in schroedinger-tests.txt, I know there are a few I missed

  • decide for or against alternate design, ala:
    An alternative design would be to refactor to use

// +build deterministic

tags for all other *_test.go files which ARE expected to pass consistently, and then use a smaller bash script to run whole files at a time in a loop. AFAIK this tagging verbosity is a limitation of go build tags and inclusion/exclusion rules. It will also mean that without http://github.com/ETCDEVTeam/go-schroedinger a lot of tests will need to be run redundantly and will thus take up a lot more time (since schroedinger picks only tests that fail from packages and reruns those individually)or will take a lot of hardcoding probably-nondeterministic tests and still some redundancy, eg.

while [[ ! go test ./eth/downloader/... ]]; then
  # This re-runs a lot of tests that already passed and take a long time to run anyways.
  echo Trying again...
done

rel #245
importantly related: https://github.com/etcdevteam/go-schroedinger

@whilei whilei requested review from sorpaas, r8d8 and tzdybal and removed request for sorpaas March 23, 2018 18:41
@whilei whilei changed the title [WIP] Fix/schroedinger tests [WIP] "Fix"/schroedinger tests Mar 24, 2018
@whilei whilei changed the title [WIP] "Fix"/schroedinger tests "Fix"/schroedinger tests Mar 24, 2018
@tzdybal
Copy link
Contributor

tzdybal commented Mar 26, 2018

To make testing more granular and explicit, we can also rename (prefix or suffix) all non-deterministic tests and use -run parameter with appropriate regexp. What do you think about it @whilei, @r8d8?

@tzdybal
Copy link
Contributor

tzdybal commented Mar 26, 2018

From go 1.10 passing tests are cached by default. So they won't be executed multiple times. The most KISS solution is to rerun all tests (cache is salvation) in a loop.

@whilei
Copy link
Contributor Author

whilei commented Mar 27, 2018

Thinking toward explicitness and granularity... we can build that list with a couple of seds from your (@tzdybal) previous analysis of (non)determinism rates and occurences using -count, yea?

The prefix/suffix + -run <regex> idea would allow us to include or exclude tests as desired... but still doesn't give us much granular control over re-running only individual failing tests, or? I'm imagining it would look something like:

# pseudocode
go test -tags="sputnikvm deterministic" -v ./...
allowedTrials=4
trials=0
while [[ ! go test -tags='sputnikvm !deterministic' -v ./... -run "(.*Schroedinger$)" ]]; do
    trials++
    if [[ trials > allowedTrails ]];
       break;
    fi
done

Copy link
Contributor

@tzdybal tzdybal left a comment

Choose a reason for hiding this comment

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

It's not ideal solution, but will save us hours ;)

@whilei
Copy link
Contributor Author

whilei commented May 2, 2018

@tzdybal -- I'm not completely sure of your mod 40a8e56 because I'm not familiar with the circle ci workflow setups -- but basically LGTM if LGTY

@whilei
Copy link
Contributor Author

whilei commented May 2, 2018

@tzdybal Tag, you're it; your merge if LTGY. LGTM

Get used to green. 💚

@tzdybal tzdybal force-pushed the fix/schroedinger-tests branch from 5b7b2d7 to 671a3e1 Compare May 3, 2018 00:05
@tzdybal
Copy link
Contributor

tzdybal commented May 3, 2018

I have to add my glog rotation tests to the shame-list 👎

@whilei
Copy link
Contributor Author

whilei commented May 7, 2018

@tzdybal Do you want to add them before merge, or separately after?

@tzdybal
Copy link
Contributor

tzdybal commented May 7, 2018

I created an issue for this and I want to fix them.

@whilei
Copy link
Contributor Author

whilei commented May 7, 2018

Ah, gotcha -- #564

@tzdybal
Copy link
Contributor

tzdybal commented May 7, 2018

See: #580.

So let's merge this PR.

@tzdybal tzdybal merged commit 671a3e1 into master May 7, 2018
@soc1c soc1c deleted the fix/schroedinger-tests branch June 19, 2019 12:30
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