Skip to content
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

Basic GitHub actions-based CI #1402

Merged
merged 11 commits into from
Mar 17, 2021
Merged

Basic GitHub actions-based CI #1402

merged 11 commits into from
Mar 17, 2021

Conversation

gdeest
Copy link
Contributor

@gdeest gdeest commented Mar 14, 2021

(New version of #1396 from a Servant repository branch, in order to get GitHub actions actually running here.)

The Travis CI has been broken for a while, and it is probably time to fix it so that proper development can continue.

This PR:

  • Setups a basic CI based on GitHub actions, with a somewhat limited build matrix.
  • Disables cookbook/testing, because servant-quickcheck doesn't build anymore.
  • Disables servant-docs on Cabal build, because of some test failures
    • The order of some JSON fields seems to be reversed in the output, need investigation.
  • Fix test failures in servant-http-streams when localhost points to an IPv6 address rather than 127.0.0.1.

If this PR is merged, I plan on fixing and re-enabling all the disabled tests one-by-one. I think the first step here is to get back to a green CI to prevent further regressions.

Opening as draft for now because it still needs a bit of cleanup. An example run can be witnessed here:

https://github.com/gdeest/servant/runs/2024096343

@gdeest
Copy link
Contributor Author

gdeest commented Mar 15, 2021

For the record, here is what seems to be missing ATM:

  • Support for additional GHC versions (it's just a matter of adding them to the build matrix, and potentially fixing associated issues).
  • GHCJS tests for the relevant parts of the codebase.
  • Doctests.

@akhesaCaro akhesaCaro mentioned this pull request Mar 16, 2021
@gdeest
Copy link
Contributor Author

gdeest commented Mar 16, 2021

I noticed the following weirdness in servant-client tests:

Preprocessing test suite 'readme' for servant-client-0.18.2..
Building test suite 'readme' for servant-client-0.18.2..
Running 1 test suites...
Test suite readme: RUNNING...
Error: ConnectionError (HttpExceptionRequest Request {
  host                 = "localhost"
  port                 = 8081
  secure               = False
  requestHeaders       = [("Accept","application/json;charset=utf-8,application/json")]
  path                 = "/books"
  queryString          = ""
  method               = "GET"
  proxy                = Nothing
  rawBody              = False
  redirectCount        = 10
  responseTimeout      = ResponseTimeoutDefault
  requestVersion       = HTTP/1.1
  proxySecureMode      = ProxySecureWithConnect
}
 (ConnectionFailure Network.Socket.connect: <socket: 3>: does not exist (Connection refused)))
Test suite readme: PASS
Test suite logged to:
/home/runner/work/servant/servant/dist-newstyle/build/x86_64-linux/ghc-8.8.4/servant-client-0.18.2/t/readme/noopt/test/servant-client-0.18.2-readme.log

This may be another IPv6 / network-related issue. But I am puzzled by the fact that the test suite is marked as “passing” when it is clearly failing. Needs investigation. This is normal. This is a sample program in README.lhs to demonstrate basic usage of servant-client. The program tries to connect on localhost, which fails because there is no server running, and displays the error in the console. This is not a test failure.

@gdeest gdeest force-pushed the fix-ci branch 2 times, most recently from 8eea30f to 0b9b2a1 Compare March 16, 2021 10:06
@gdeest
Copy link
Contributor Author

gdeest commented Mar 16, 2021

We are hitting: sol/doctest#119

When running doctest the dumb way. There doesn't seem to be an easy way around this (though I am unsure why current setup doesn't seem affected base-compat-batteries is excluded from doctest setup, see below).

I'll give a stab at it using cabal-doctest, and writing dedicated test suites for the doctests. This seems to be the simplest way to invoke GHC with just the required dependencies.

PackageImports would work as well, but this would be quite an obtrusive change to the codebase for the sole purpose of running tests.

@@ -3,7 +3,8 @@ packages:
servant-client/
servant-client-core/
servant-http-streams/
servant-docs/
-- Tests failing with Cabal (TODO: investigate)
-- servant-docs/
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know ! There is a pending PR ( #1397 ) that fixes this problem in a slightly different way. We'll ponder what the best approach is.

@gdeest
Copy link
Contributor Author

gdeest commented Mar 16, 2021

The Travis CI works around the ambiguous module problem with doctests with:

doctest-filter-packages: base-compat-batteries

in cabal.haskell-ci. It gives me hope that this seems to be the only package requiring this ; I may be able to reproduce what current .travis.yml is doing easily in the Github setup without going down the custom setup / dedicated test-suite road (even though I think this might ultimately be better).

@gdeest gdeest force-pushed the fix-ci branch 2 times, most recently from 6ec0f00 to 0a199d2 Compare March 16, 2021 15:52
@gdeest
Copy link
Contributor Author

gdeest commented Mar 16, 2021

The doctests were actually only failing on MacOS, which happened to run faster than Ubuntu and cancelled the entire build.

Disabling the MacOS builds for now since the Travis CI doesn't have them anyway, and we're only aiming for feature-parity at this point.

@gdeest gdeest force-pushed the fix-ci branch 2 times, most recently from 029b112 to 1b3a986 Compare March 16, 2021 18:07
@gdeest
Copy link
Contributor Author

gdeest commented Mar 16, 2021

The UVerb cookbook is triggering a failure on GHC < 8.6 because of a usage of DerivingVia (which was introduced in GHC 8.6.1).

We will modify the cookbook not to use this extension.

@akhesaCaro akhesaCaro force-pushed the fix-ci branch 3 times, most recently from ee2e134 to 22755b4 Compare March 16, 2021 20:44
@gdeest gdeest force-pushed the fix-ci branch 12 times, most recently from 4b2f9b4 to cf21eaf Compare March 17, 2021 17:36
@gdeest gdeest force-pushed the fix-ci branch 3 times, most recently from 10ff95e to d3da568 Compare March 17, 2021 22:21
Gaël Deest and others added 11 commits March 17, 2021 23:22
Also remove mention about re-generating .travis.yml in README
- Setup a basic CI based on GitHub actions, with a somewhat limited build matrix.
- Disable cookbook/testing, because servant-quickcheck doesn't build anymore.
- Disable servant-docs on Cabal build, because of some test failures
  - The order of some JSON fields seems to be reversed in the output, need investigation.
- Fix test failures in servant-http-streams when `localhost` points to an IPv6 address rather than 127.0.0.1.
@gdeest
Copy link
Contributor Author

gdeest commented Mar 17, 2021

CI is fixed, and (AFAIK) has feature-parity with previous setup. Since the Travis setup is basically broken, I'll merge now.

A few trivial changes were required:

  • New cookbooks did not compile with older GHC versions, due to to the use of extensions that weren't available at that time (DerivingVia, DerivingStrategies) or to changes in the structure of base (missing imports for Data.Monoid / Data.Semigroup).
  • A test was fixed that involved a problem when localhost is resolved to ::1 instead of 127.0.0.1.

A servant-docs test was commented out for now. It is inherently buggy, as it relies on the order in which aeson will render JSON object fields, but aeson uses a HashMap from unordered-containers for representing objects. A fix is proposed in #1397.

Things to improve on:

  • Performance: CI is currently taking in the order of ~20 minutes. There is caching in place, but the cache doesn't seem to always be restored.
  • GHCJS setup. We're still using hvr's PPA which doesn't seem to be updated anymore (we're stuck with ghcjs-8.4). We could try to use Nix / reflex-platform.
  • General ugliness / hacksinherited from Travis CI.

@gdeest gdeest merged commit a74d9d9 into master Mar 17, 2021
@gdeest gdeest deleted the fix-ci branch March 17, 2021 22:50
@akhesaCaro akhesaCaro mentioned this pull request Apr 10, 2021
7 tasks
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