Skip to content

travis: Add hlint to Travis-CI checks #374

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

Merged
merged 2 commits into from
Sep 28, 2016
Merged

travis: Add hlint to Travis-CI checks #374

merged 2 commits into from
Sep 28, 2016

Conversation

rbasso
Copy link
Contributor

@rbasso rbasso commented Sep 26, 2016

  • Add hlint tests to the entire repository.
  • Add and fix comments to match that.

Related to #355.

Motivation

It was briefly discussed in #355 that it would be interesting to run HLint in Travis-CI.

Thanks to @petertseng, we discovered that hlint is already available and whitelisted in Travis, but the version available in Ubuntu Precise is older than the ones we would like to run.

Initial experiments showed us that, installing it from source, the initial building times could be problematic. Another problem would be to assure that we are running a known version of HLint.

I opened this PR as an experiment, to see - after we finish #355 - if it is feasible to install and run it in "production". It also serves to check if we are really done with fixing the current code base.

The general idea is to install hlint using the same Stack snapshot used for testing the exercises, increasing Stack cache sharing while using a HLint version known to compile without problems.

I added HLint tests between configlet and the exercises' test. This way we can get feedback really fast if we push something without running hlint before.

Preliminary results

Installing hlint but not running it:

  • Initial building time: 27m31s
  • Rebuilding time: 1m50s
  • Approximate cache size per snapshot: 700MB

I had problems in my first tests, but I was not able to study the logs.
As soon as I get better Internet connection I'll go back to it.

I think we'll be able to put that in production soon @petertseng. Thanks again!

@rbasso
Copy link
Contributor Author

rbasso commented Sep 26, 2016

@petertseng, what do you think about this PR?

  • Should we run hlint before the exercises, before each of the exercises, after each exercise or after all the exercises?
  • Do you agree with using the hlint from the same Stack snapshot (for the current snapshot I think it is v1.9.35)?
  • Do you think those initial build times are acceptable?

@rbasso
Copy link
Contributor Author

rbasso commented Sep 26, 2016

Additional tests

I just run some more tests clearing the caches in my fork and temporarily disabling the error code from hlint.

Branch: master
    Building time (without cache)
        nightly-2016-07-17 :  14m12s
        nightly            :  14m36s

    Rebuild time (with cache)
        nightly-2016-07-17 :   1m26s
        nightly            :   1m50s

    Total cache size       : 1346.28MB

Branch: travis-hlint-no-exit-code

    Building time (without cache)
        nightly-2016-07-17 :  21m03s
        nightly            :  21m09s

    Rebuild time (with cache)
        nightly-2016-07-17 :   1m41s
        nightly            :   2m03s

    Total cache size       : 1384.49MB

Mean variation:
    Building time          : + 6m42s
    Rebuild time           : + 0m14s
    Tocal cache size       : + 38.21MB

It seems that the only relevant cost in installing and running hlint is the initial build time.

Considering that we don't usually clear the cache, those 7 additional minutes don't look so bad to me. I think we can still follow the plan of running it on Travis.

If in the future those 7 minutes prove to be critical, we can easily revert the change. 😄

@petertseng
Copy link
Member

Do you think those initial build times are acceptable?
Considering that we don't usually clear the cache, those 7 additional minutes don't look so bad to me. I think we can still follow the plan of running it on Travis.

Yes, since it is cached we should be OK!

Do you agree with using the hlint from the same Stack snapshot (for the current snapshot I think it is v1.9.35)?

It is best if we do this, since it is best to be able to get the same result locally as that Travis will give us. So I support using Stack.

Should we run hlint before the exercises, before each of the exercises, after each exercise or after all the exercises?

I remember a related conversation at some point as to whether we want to run configlet first or last, but I don't have the conversation handy right now nor can I search for it. We wanted to fail fast then, right? hlint runs pretty fast, and faster than the individual tests, I think. That seems to support running it before. I don't actually feel strongly though.

- Add hlint tests to the entire repository.
- Add and fix a few comments.
@rbasso
Copy link
Contributor Author

rbasso commented Sep 28, 2016

Now that we finished #355, I think we are ready to add hlint to Travis.

Everything seems fine in Travis-CI: https://travis-ci.org/exercism/xhaskell/builds/163280543

@rbasso rbasso changed the title travis: Add hlint to Travis-CI checks. (Experimental - DO NOT MERGE IT) travis: Add hlint to Travis-CI checks Sep 28, 2016
@rbasso
Copy link
Contributor Author

rbasso commented Sep 28, 2016

Added commit updating README.md to explain that hlint now runs on Travis-CI.

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

yup, looks good. One small change (I can change it when I have the time, but not right now)

a *pull request*, so you can fix your code before submitting it for review.

If you are certain that a suggestion given by `hlint` would make the
code worse, you can [supress it](https://github.com/ndmitchell/hlint#customizing-the-hints)
Copy link
Member

Choose a reason for hiding this comment

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

suppress

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@petertseng petertseng merged commit f6a5262 into exercism:master Sep 28, 2016
@rbasso rbasso deleted the travis-add-hlint branch September 28, 2016 10:26
highly recommended to run `hlint` on your sources before opening a
*pull request*.
All code in this repository should be as idiomatic as possible, so we
enforce in [Travis-CI] that it returns `No hints` when processed by
Copy link
Member

Choose a reason for hiding this comment

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

Should this [Travis-CI] have some (URL) after it so that it becomes a link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops! forgot to remove the brackets.

Should we add a links to https://travis-ci.org/exercism/xhaskell when mentioning Travis? I think we have two references to it.

Copy link
Member

Choose a reason for hiding this comment

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

We do have the build status image, so it wouldn't strictly be necessary. You're right that we mention Travis multiple times in this file. If we want to link it I suggest we do something to only have to link to it once:

[e 1][ex]
[e 2][ex]

[ex]: http://example.com

e 1
e 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a PR removing the brackets to fix the problem.

I think that the README deserves a rewrite, but I'm without ideas for now.

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