Skip to content

Conversation

@jhrcek
Copy link
Contributor

@jhrcek jhrcek commented Oct 6, 2019

Description of the change

Attempt to speed up test suite by focusing on the slowest tests as described in #440

Locally this change seems to have shaved off ~20s of test running time.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

@jhrcek jhrcek requested review from Dretch and f-f October 6, 2019 08:03
Copy link
Contributor

@Dretch Dretch left a comment

Choose a reason for hiding this comment

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

Looks good to me.


spago ["bump-version", "minor"] >>= shouldBeSuccess
getHighestTag >>= (`shouldBe` Just "v1.2.3")
-- TODO should this work or should we strip these in parser implementation?
Copy link
Contributor

Choose a reason for hiding this comment

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

PREREL / META should probably be allowed - for one thing because there are existing packages that have such versions - e.g. halogen v5.0.0-rc.7.

parseVersionBump "1.2.3" `shouldBe` Just (Exact (SemVer 1 2 3 [] []))

before_ (initGitTag "v1.2.3") $ it "Spago should not make a tag when not passing --no-dry-run" $ do
-- TODO is this desired behavior, or should we just drop ONE 'v'? I'd agree it's edge case, but still :-)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know. 🤷‍♂️

@Dretch Dretch self-requested a review October 6, 2019 13:57
@jhrcek
Copy link
Contributor Author

jhrcek commented Oct 7, 2019

@f-f just wanted to check with you what's the general strategy around PR reviews. Should I wait for your review/approval before I merge, or is just one review enough, like in the case of this PR?

@f-f
Copy link
Member

f-f commented Oct 7, 2019

@jhrcek great point, thanks for asking. This has not come up before so that's why we don't have a written policy about it yet

I think a review from a contributor is enough to merge, no need to wait for my review :)

@jhrcek jhrcek merged commit d735d90 into purescript:master Oct 7, 2019
@jhrcek jhrcek mentioned this pull request Oct 7, 2019
@f-f
Copy link
Member

f-f commented Oct 7, 2019

@jhrcek thanks for doing this 🙂

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.

3 participants