Use Github Action to replace Travis and AppVeyor for CI.

[wyk9787]

TL;DR

Things included in this PR:

• Added a Github Action script draft (macOS, Linux and Windows test).

Follow-up plan:

• Verify the correctness of the draft.

• Remove working {Go, OS, XCode} sections that are covered by this Github Action script from existing CI scripts.

• Add build badge to README.md.

• Enable Github Action as part of PR review process.

Details

Why it is only a draft?

The Github Action script (i.e. ci.yaml) is first introduced in this PR. That means any change made to ci.yaml can be only tested on the forked repo (i.e. it will trigger the Github Action on the forked repo if a commit is pushed to the master branch). This seems to be the intentional behavior as noted here.

Limitations

1. The master branch of Go is not supported by setup-go. There is an open PR that is working on this: Add support for gotip and latest actions/setup-go#34, but no progress has been made since March and no ETA.

2. Only the latest macOS is supported: https://help.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idruns-on.

3. Limited number of XCode versions are supported: https://github.com/actions/virtual-environments/blob/master/images/macos/macos-10.15-Readme.md#xcode, but this should not be an issue for us as we wouldn't support anything older than XCode 10.3.

Solution

We will keep the tests for {GO, OS, XCode} combination that are not currently supported by Github Action in existing CI scripts. Ideally, we will only need to maintain one CI.

[codecov-commenter]

Codecov Report

Merging #541 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #541   +/-   ##
=======================================
  Coverage   68.33%   68.33%           
=======================================
  Files          39       39           
  Lines        8044     8044           
=======================================
  Hits         5497     5497           
  Misses       2120     2120           
  Partials      427      427           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03e1cf3...5c6d24e. Read the comment docs.

[aalexand]

The series of commits with "Update to test" message is not very useful. Either make commit messages useful or squash the commits and "push -f".

[aalexand]

The coverage report seems to say that the coverage has dropped - why?

[wyk9787]

The series of commits with "Update to test" message is not very useful. Either make commit messages useful or squash the commits and "push -f".

This PR is still work in progress; there is "DO NO REVIEW" in PR description. I thought I needed to open the PR to test something. I will squash those commits once it is ready for review.

[aalexand]

The Travis CI build is marked as red. Should we disable this? Also, can we mark the new CI configuration as required to pass for any PR to be submitted? We want to ensure that only green testing status is allowed to be submitted.

[wyk9787]

The Travis CI build is marked as red. Should we disable this? Also, can we mark the new CI configuration as required to pass for any PR to be submitted? We want to ensure that only green testing status is allowed to be submitted.

The Travis CI still suffers from the timeout issue. We should disable this and remove travis.yml once we verify Github action is working correctly. As far as this PR, we should just keep retrying the Travis test to make it pass. Starting from next PR, which I will create once this one gets merged, we can get rid of Travis and start using Github Action.

Once this PR gets merged, I think admin (I don't have that access) is able to mark the new CI configuration as required to pass for any PR to be submitted in this repo's settings.

[kalyanac]

I think Travis CI uses 2 versions on MacOS, Ubuntu 16.04 LTS.
We should run on Ubuntu latest, and LTS at least. Similarly, latest 2 versions on macOS with latest 2 versions on Xcode.

[wyk9787]

Added 18.04 and 20.04. "ubuntu-latest" is 18.04. There is no support for other versions of macOS besides the latest, unfortunately. See here.

I have spent the past two hours trying to mess around the keywords "include" and "exclude" as documented here. I am not sure this is a bug or it's the intended behavior, but basically there is no elegant way for me to say I want the additional following two combinations: os = 'macos-latest', xcode-version = ['1.13', '1.14'].

It's pretty clear from this example that you are able to introduce a new variable here. But first of all you are not able to say:

include:
  - os: 'macos-latest'
     xcode-version: ['latest', '10.3']

because of the syntax issue (i.e. it does not support a list here).

But then you are not able to say

include:
  - os: 'macos-latest'
     xcode-version: 'latest'
  - os: 'macos-latest'
     xcode-version: '10.3'

either, since the second one (i.e. 10.3) will simply override the first one.

The final outcome is a compromise where I don't use "include" or "exclude" keywords at all and simply let all of them run. This will result 12 runs (see here), where 4 of them are repeated (i.e. 2 ubuntu versions * 2 xcode versions). They will still pass without issues but those 4 runs are kind of wasted.

Given the time constraints on Github Action is really not an issue (with 12 runs, they finished in less than 5 minutes), I think this is the best we can do. I do notice Github Action is rapidly developing (e.g. this whole matrix.strategy thing is introduced not too long ago), so maybe there is hope afterall.

[nolanmar511]

If I recall correctly, there are either limits on the number of GitHub actions workflows or they're paid; also, more tests creates more opportunity for flakiness, and the listing of tests run might be somewhat confusing.

It might be worth some repetition (specifically, splitting out Mac and Linux test cases) to avoid this.

[aalexand]

I'd be unhappy to lose testing against Go master branch.

[wyk9787]

I'd be unhappy to lose testing against Go master branch.

One compromise we can make while we are waiting for actions/setup-go#34 to happen is to keep the Travis CI but only testing against Go master branch. Supposedly, timeout should not be an issue in this case if we remove other Go versions.

[aalexand]

I'd be unhappy to lose testing against Go master branch.

One compromise we can make while we are waiting for actions/setup-go#34 to happen is to keep the Travis CI but only testing against Go master branch. Supposedly, timeout should not be an issue in this case if we remove other Go versions.

There isn't recent progress on actions/setup-go#34 it seems. We should probably ping it to try to get an estimate on whether it's coming and when.

[wyk9787]

@kalyanac @wyk9787 Does the PR description accurately reflect the work done in this PR and the future plan?

Yes. I added one last sentence "...remove {Go, OS, XCode} sections that are covered by this Github action script." to better describe the next move.

[wyk9787]

@aalexand Is it okay to merge this PR?

[aalexand]

@aalexand Is it okay to merge this PR?

Sorry, this might be just me, but I can't easily grok the full plan from the PR description. It's long and contains many details, but what I'm looking for to understand is a summary of:

• What this PR does and why.
• What this PR doesn't do (i.e. the delta from the desired state), why and when that's going to be done.

Additional details are good, but the above is what I'm looking for.

Thanks for bearing with me :)

[wyk9787]

@aalexand Is it okay to merge this PR?

Sorry, this might be just me, but I can't easily grok the full plan from the PR description. It's long and contains many details, but what I'm looking for to understand is a summary of:

• What this PR does and why.
• What this PR doesn't do (i.e. the delta from the desired state), why and when that's going to be done.

Additional details are good, but the above is what I'm looking for.

Thanks for bearing with me :)

Yes, I agree. It was definitely a bit messy.

Modified description to have sections and emphasis the "done" and "to-do".

[aalexand]

@aalexand Is it okay to merge this PR?

Sorry, this might be just me, but I can't easily grok the full plan from the PR description. It's long and contains many details, but what I'm looking for to understand is a summary of:

• What this PR does and why.
• What this PR doesn't do (i.e. the delta from the desired state), why and when that's going to be done.

Additional details are good, but the above is what I'm looking for.
Thanks for bearing with me :)

Yes, I agree. It was definitely a bit messy.

Modified description to have sections and emphasis the "done" and "to-do".

Thanks. Do I get it correctly that once we test it and it works, we should be able to at least get rid of the Appveyor config?

[wyk9787]

@aalexand Is it okay to merge this PR?

Sorry, this might be just me, but I can't easily grok the full plan from the PR description. It's long and contains many details, but what I'm looking for to understand is a summary of:

• What this PR does and why.
• What this PR doesn't do (i.e. the delta from the desired state), why and when that's going to be done.

Additional details are good, but the above is what I'm looking for.
Thanks for bearing with me :)

Yes, I agree. It was definitely a bit messy.
Modified description to have sections and emphasis the "done" and "to-do".

Thanks. Do I get it correctly that once we test it and it works, we should be able to at least get rid of the Appveyor config?

I certainly hope so but I am not too confident. See issue no. 2 in the last section.

[aalexand]

Thanks. Do I get it correctly that once we test it and it works, we should be able to at least get rid of the Appveyor config?

I certainly hope so but I am not too confident. See issue no. 2 in the last section.

I don't get the diff there - what's different? Visually these are the same, am I missed something? Or, line endings?..

[wyk9787]

Thanks. Do I get it correctly that once we test it and it works, we should be able to at least get rid of the Appveyor config?

I certainly hope so but I am not too confident. See issue no. 2 in the last section.

I don't get the diff there - what's different? Visually these are the same, am I missed something? Or, line endings?..

I don't get the diff either :)
But somehow the unit tests fail with this error message, showing me the diff that is the same.

[aalexand]

Thanks. Do I get it correctly that once we test it and it works, we should be able to at least get rid of the Appveyor config?

I certainly hope so but I am not too confident. See issue no. 2 in the last section.

I don't get the diff there - what's different? Visually these are the same, am I missed something? Or, line endings?..

I don't get the diff either :)
But somehow the unit tests fail with this error message, showing me the diff that is the same.

So line endings?

[wyk9787]

Thanks. Do I get it correctly that once we test it and it works, we should be able to at least get rid of the Appveyor config?

I certainly hope so but I am not too confident. See issue no. 2 in the last section.

I don't get the diff there - what's different? Visually these are the same, am I missed something? Or, line endings?..

I don't get the diff either :)
But somehow the unit tests fail with this error message, showing me the diff that is the same.

So line endings?

FYI, @kalyanac and @nolanmar511

You are right.
After learning Windows dev 101 (very fun indeed), I was finally able to reproduce the error on a Windows VM.
It turns out when using https://golang.org/pkg/io/ioutil/#ReadFile to read test file, it will add a "\r" in addition to "\n" at the end of the line on Windows. It makes sense since on Windows, "\r\n" together indicates a new line.

However, any pprof output (e.g. a dot file) does not include "\r", and that's why we are seeing the "weird" diff mismatch.

ioutil.ReadFile is widely used within pprof tests. Any immediate idea on how this issue can be easily addressed?

[aalexand]

ioutil.ReadFile is widely used within pprof tests. Any immediate idea on how this issue can be easily addressed?

strings.ReplaceAll(s, "\r\n", "\n") in the Diff() function used in the tests seems the most straightforward fix perhaps.

[aalexand]

It turns out when using https://golang.org/pkg/io/ioutil/#ReadFile to read test file, it will add a "\r" in addition to "\n" at the end of the line on Windows. It makes sense since on Windows, "\r\n" together indicates a new line.

I am not sure that ReadFile has that logic. I would expect that what happens is that when pprof prints a line it uses "\n" which on Windows expands to "\r\n". So the golden files read from disk have just "\n", but the reference, pprof-generated output has "\r\n". If ReadFile would do the expansion, the test would pass since both golden and reference files are read from disk.

So I think you can ignore my previous suggestion and just add the line endings replacement here -

pprof/internal/driver/driver_test.go

Line 196 in 03e1cf3

sbuf = bytes.Replace(sbuf, []byte("/path/to/"), []byte("\\path\\to\\"), -1)

.

[aalexand]

Is it possible to add an icon / thumbnail on the main page to indicate the status of this new CI build, like we have for Travis and Appveyor?

[wyk9787]

It turns out when using https://golang.org/pkg/io/ioutil/#ReadFile to read test file, it will add a "\r" in addition to "\n" at the end of the line on Windows. It makes sense since on Windows, "\r\n" together indicates a new line.

I am not sure that ReadFile has that logic. I would expect that what happens is that when pprof prints a line it uses "\n" which on Windows expands to "\r\n". So the golden files read from disk have just "\n", but the reference, pprof-generated output has "\r\n". If ReadFile would do the expansion, the test would pass since both golden and reference files are read from disk.

So I think you can ignore my previous suggestion and just add the line endings replacement here -

pprof/internal/driver/driver_test.go

Line 196 in 03e1cf3

sbuf = bytes.Replace(sbuf, []byte("/path/to/"), []byte("\\path\\to\\"), -1)

.

I was actually looking at the failed tests in internal/graph/.... I was printing the ascii values from both got and want in

pprof/internal/graph/dotgraph_test.go

Line 190 in 03e1cf3

func compareGraphs(t *testing.T, got []byte, wantFile string) {

using this function:

func printAscii(chunk []byte) {
  fmt.Println(“START”)
  for _, b := range chunk {
    fmt.Printf(“%d “, b)
  }
  fmt.Println(“DONE”)
}

It was actually want (golden files read from disk) which has 13 ("\r"'s ascii value) followed by a 10 ("\n"'s ascii value) in the output. got (output generated by pprof, and was not written to a file in this case) only has 10 in the out.

But you are right, for tests in driver_test.go, pprof generated outputs are first written to the disk and then read from disk using ReadFile(). So something else is going on here.

strings.ReplaceAll(s, "\r\n", "\n") in the Diff() function used in the tests seems the most straightforward fix perhaps.

This will not cut it here since Diff() is called after the bytes comparison:

pprof/internal/graph/dotgraph_test.go

Line 197 in 03e1cf3

if string(got) != string(want) {

.

[wyk9787]

Is it possible to add an icon / thumbnail on the main page to indicate the status of this new CI build, like we have for Travis and Appveyor?

Yes, see the third bullet point of "follow-up plan" in PR description.

Will add it in the next PR following https://help.github.com/en/actions/configuring-and-managing-workflows/configuring-a-workflow#adding-a-workflow-status-badge-to-your-repository.

[aalexand]

It was actually want (golden files read from disk) which has 13 ("\r"'s ascii value) followed by a 10 ("\n"'s ascii value) in the output. got (output generated by pprof, and was not written to a file in this case) only has 10 in the out.

I guess you need to look at whether it's Git here that replaces the line endings on checkout then.

[wyk9787]

It was actually want (golden files read from disk) which has 13 ("\r"'s ascii value) followed by a 10 ("\n"'s ascii value) in the output. got (output generated by pprof, and was not written to a file in this case) only has 10 in the out.

I guess you need to look at whether it's Git here that replaces the line endings on checkout then.

I would have never thought it was actually Git that changes the line endings. Thanks!
Added a .gitattributes file to address this issue. Now tests are passing: https://github.com/wyk9787/pprof/actions/runs/147825124.

Also modified PR description.

[aalexand]

@wyk9787 For the internal package import errors: in Travis config we use go_import_path: github.com/google/pprof setting to overcome that. Is there something similar in GH actions? If not, how are others dealing with this? It would be useful to figure this out, otherwise we won't know that the tests pass when someone sends us a PR?

[aalexand]

@wyk9787 For the internal package import errors: in Travis config we use go_import_path: github.com/google/pprof setting to overcome that. Is there something similar in GH actions? If not, how are others dealing with this? It would be useful to figure this out, otherwise we won't know that the tests pass when someone sends us a PR?

@wyk9787 https://laszlo.cloud/setting-gopath-in-github-actions#controlling-the-checkout-path ?

[wyk9787]

@wyk9787 For the internal package import errors: in Travis config we use go_import_path: github.com/google/pprof setting to overcome that. Is there something similar in GH actions? If not, how are others dealing with this? It would be useful to figure this out, otherwise we won't know that the tests pass when someone sends us a PR?

@wyk9787 https://laszlo.cloud/setting-gopath-in-github-actions#controlling-the-checkout-path ?

I was using a custom path for checkout but I included a variable GITHUB_REPOSITORY, which will lead to "wyk9787/pprof" for my testing branch. I thought the package checking is more than checking the path, so I didn't realize this was the issue.

Your comments reminded me to change it to hardcoded "google/pprof" in the path and it works now :)
https://github.com/wyk9787/pprof/runs/809235334?check_suite_focus=true

Thanks again for helping with both of those issues!