GH Actions CI #883
GH Actions CI #883
Conversation
…o fast and loose :-)
* Needed because was match very explicitly on headers
|
Sorry to open a new PR about this, but the other one was long enough ago that I completely re-imagined how I wanted to do this. I hope this makes sense to y'all too. Things we talked about that I want to highlight:
That's the spirit I went with, which means stuff like
I omitted because it is going to break and that probably deserves its own PR.
Totally agreed, this should be relatively straightforward to add in a followup PR. Though AFAICT the server running on another thread isn't part of |
|
Okay, so what do we need to get this merged? If we have to strip stuff out that's fine, we can always work on readding those things, but I would love a list of things we're leaving behind. |
I also would like to drop the entire Edit: Sorry, as mentioned in the description we're also now skipping a flakey test...I'm on the side of merge and then fix later...but later often means never 😢 |
|
@olleolleolle @krainboltgreene gentle bump to see if there's anything I can do to help to move this forward (and apologies for the direct mentions if they prove less than helpful) 😄 |
| on: | ||
| push: | ||
| branches: [ $default-branch ] | ||
| pull_request: |
There was a problem hiding this comment.
I'm curious if this alias for default branch does the right thing. It's be cool.
There was a problem hiding this comment.
In order to keep the momentum, @bradshjg, let's merge this, which added a single defined skip, which can be investigated. Warnings are collected and analyzed.
Em-http-related testing fixes for reproducibility could potentially be a separate thing, but I'm OK with those spec changes landing at the same time.
I had a tiny note about the $default-branch name, which is easy to see if it worked or not, after a merge. Otherwise, we can change it to the relevant string before merge. Your call, @bradshjg. Update: I made the change to an explicit literal string with the default branch name in it.
In all, this looks like future to me. That is, 👍 !
|
@bradshjg Hooray! Thank you so much for your hard work in creating this! |
Supercedes #881
Related to #851
This represent a fairly drastic change in how tests are run, previously a single script
ci.shwas responsible for running all the steps and appropriately timing and folding the relevant sections. This PR splits the CI steps into separate Actions steps,but doesn't preserve at least one (probably important!) behavior: testing for warnings.Edit: this now does check for warnings but the semantics might differ slightly fromci.sh.It's not clear to me whether we want to dump all warnings in various steps to a file or create a similar abstraction asEdit: each test step dumps stderr to a file and then that file is checked for warnings in a later step.ci.shhad where warning checks were sorta built-in to the various steps. I found the indirection inci.shpainful to follow and personally would probably prefer more explicit (if duplicated)2> {some_warnings_file}and then a step to crawl through those at the end, but I'm totally open to suggestions.Other potentially controversial things:
em-http-requestadds headers by default, and in the test suite there's very reasonable header tests that get broken by this behavior. I opted to update the request options in a way that omits those headers so the tests pass as written, but it's kinda weird and fragile but hey they started it! 😄