Skip to content

Enable Trace2 logging of functional tests#482

Merged
jeffhostetler merged 3 commits intomicrosoft:mainfrom
jeffhostetler:ci-with-trace2
Jan 22, 2021
Merged

Enable Trace2 logging of functional tests#482
jeffhostetler merged 3 commits intomicrosoft:mainfrom
jeffhostetler:ci-with-trace2

Conversation

@jeffhostetler
Copy link
Contributor

Enable Trace2 logging in Git for the functional test suite.
Then zip up the logs and store in as a single artifact.

mjcheetham and others added 3 commits January 22, 2021 04:30
Force the installation of watchman and any dependencies in the macOS
installation script. This helps get around issues whereby dependencies
of watchman (looking at you Python) complain during installation that
some extra tools were not linked (unless you specify --force), and the
script fails overall.

Also set ownership of several Homebrew directories which may be broken,
and remove any errant 2to3 Python binaries that may exist in
/usr/local/bin, which would cause linking issues when installing
watchman's Python 3 dependency.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
…processes

Allow Trace2 environment variables to pass thru to child processes.

A change was made last year to strip out all GIT_TRACE* variables
from the environment of child processes to prevent them from writing
to stderr and confusing the parent process (which is only expecting
legitimate Git error output).  This filtering could also capture Trace2
output directed to a log file (because Trace2 has more settings than
traditional tracing).

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
@jeffhostetler
Copy link
Contributor Author

This PR causes the CI builds to each temporarily archive about 35Mb (per target) of Trace2 data. I have it set to expire them in 3 days.

Wondering if this should be an optional feature (as in should we have a feature flag defaulting to false at the top and we can add a temporary commit to turn it on when desired) (or if Actions has an official way of doing this).

A quick scan of the Trace2 output shows that the functional tests generates over 200 Git errors that are (I'm assuming) silently being ignored, such as ambiguous argument errors, unknown branch errors, and nonexistent blobs. So we should set aside some time to dig into them at some point and make sure that the C# tests are valid.

@derrickstolee
Copy link
Contributor

This PR causes the CI builds to each temporarily archive about 35Mb (per target) of Trace2 data. I have it set to expire them in 3 days.

Wondering if this should be an optional feature (as in should we have a feature flag defaulting to false at the top and we can add a temporary commit to turn it on when desired) (or if Actions has an official way of doing this).

I thought we had a way of storing the trace2 logs only when the build fails, so we could use the traces to diagnose the problems. (This might only be in the Azure Pipelines builds for VFS for Git.) Would this conditional export be good enough?

A quick scan of the Trace2 output shows that the functional tests generates over 200 Git errors that are (I'm assuming) silently being ignored, such as ambiguous argument errors, unknown branch errors, and nonexistent blobs. So we should set aside some time to dig into them at some point and make sure that the C# tests are valid.

Most of our functional tests compare the behavior in a vanilla Git repo with a Scalar repo. The tests continue to pass if they behave the same. It might be good to do an audit, but who has the time? Something to consider.

run: Compress-Archive -DestinationPath ${{ env.TRACE2_BASENAME }}.zip -Path ${{ env.TRACE2_BASENAME }}

- name: Archive Trace2 Logs
if: ( success() || failure() ) && ( steps.trace2_zip_unix.conclusion == 'success' || steps.trace2_zip_windows.conclusion == 'success' )
Copy link
Member

Choose a reason for hiding this comment

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

Piggy-backing on @derrickstolee's comment: should this really be also in success()' case? If you want to compare between successful runs, that's totally okay, of course. Otherwise, we may save some space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was fighting with Actions here (or maybe I inherited this from the Azure version). The ( success() || failure() ) part looks weird, but I found that if I didn't include one of them in the expression, then there was an implicit success() that would be inherited (or something like that). So this was intended as a "I want the logs without regard to whether all the functional tests passed".

@jeffhostetler
Copy link
Contributor Author

@derrickstolee I didn't see any way to get the trace2 logs here. It may have been on Azure, but I'm not sure any more. Last year I added code to log the C# unit test name and other identifying info as T2 data strings to let us sync up the output; that may be what you're thinking about. This past summer I was working on adding code here to actually capture the logs as artifacts, but got context-switched away before I could finish it.

WRT only logging on failures, I'm interested in both cases. The failures for obvious reasons. And the successes let me look at the timings and the amount and kind of data being returned. That is, I can see if fsmonitor was actually invoked and if it returned non-trivial results to the parent Git command. I could see later adding post processing steps to this script to measure and/or verify some of this, but that's later.

@jeffhostetler jeffhostetler merged commit cdbb6e7 into microsoft:main Jan 22, 2021
@jeffhostetler jeffhostetler deleted the ci-with-trace2 branch January 22, 2021 15:39
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.

4 participants