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

Modify PrepFunctionalTests.sh to work out of the output directories #427

Merged
merged 2 commits into from
Oct 26, 2018

Conversation

jrbriggs
Copy link
Member

Simplify PrepFunctionalTests by offloading determining exactly which version to install to the build script. PrepFunctionalTests will now assume the one-and-only dmg it finds in $VFS_PUBLISHDIR/Git is the version that needs to be installed to run the tests for $VFS_PUBLISHDIR

This refactoring will help support creating a minimal "functional test enlistment" for Mac.

@jrbriggs jrbriggs requested review from sanoursa and nickgra October 25, 2018 17:23
Copy link
Contributor

@sanoursa sanoursa left a comment

Choose a reason for hiding this comment

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

This is a nice simplification of the script, but I think it could complicate other things.

I'm not really clear on what a "minimal functional test enlistment" is either. Can you elaborate on that?

rm -rf $GITPUBLISH
fi
mkdir $GITPUBLISH
cp $GITPATH $GITPUBLISH
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary to actually copy? Couldn't we just update the init script to set the GITPATH variable appropriately?

Two concerns with this:

  • build+publish+unit tests is already getting to be too long. Adding a copy of a large package is just going to add to that. Maybe do the copy only if it's not already there?
  • The VFS_PUBLISHDIR currently contains just the outputs of dotnet publish GVFS.sln. With this change, that publish folder will now contain different contents if I build+publish from VS or build+publish with this script. So if we do need to copy, I'd say copy the Git package to a parallel folder, not inside the same publish folder.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't we just update the init script to set the GITPATH variable appropriately?

I'm not a fan of that, since it means we need to sweep up the packages (which I view as build input) and pass it between jobs. I think that if we need something in a package available to end users, the build should promote it to an output folder.

build+publish+unit tests is already getting to be too long. Adding a copy of a large package is just going to add to that. Maybe do the copy only if it's not already there?

Makes sense to me. I'll make that change.

So if we do need to copy, I'd say copy the Git package to a parallel folder, not inside the same publish folder.

Also makes sense. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

To add one more element here - today I can do a clean build+publish from VS, then run the prep script, and then run the functional tests.

With this change, we would now require that the build happen via this build script, at least once. It would be nice to keep the ability to build from VS if possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sanoursa and I agreed offline that, unfortunately, clean build+publish from VS already doesn't work, and the build script is already a prereq. I'm adding to the problem here. We'll move more of this into the build phase itself in subsequent refactorings, but it's deferred while we work on moving to yaml pipelines.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me. Note that I plan to spend some time next week digging into getting our build working well in VSMac (or at least understanding why things don't work well today).

@jrbriggs
Copy link
Member Author

I'm not really clear on what a "minimal functional test enlistment" is either. Can you elaborate on that?

If you look at https://gvfs.visualstudio.com/ci/_build/results?buildId=2699&view=logs notice how the functional test job doesn't build anymore. It instead sucks down the binaries it needs to run the tests on Windows. I'm starting to restructure the mac code to enable the same thing.

Once we have that working, we can teach the test runner to run one slice and let azure pipelines distribute the job across multiple agents, so we can parallelize the functional test runs.

@sanoursa
Copy link
Contributor

@jrbriggs Yep I've seen that but I'm still a bit fuzzy on it. Maybe I'm just hung up on the word "enlistment". Would it be accurate to say you're just publishing the build artifacts from one job, and then consuming them from another?

@jrbriggs
Copy link
Member Author

@sanoursa yes, that's exactly it. It's an "enlistment" insofar as the layout is structured, but I'm happy to drop that verbage.

mkdir $GITPUBLISH
fi

find $GITPUBLISH -type f ! -name $INSTALLER -delete
Copy link
Member

Choose a reason for hiding this comment

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

Is this line removing any old installers that may be present?

Copy link
Member Author

@jrbriggs jrbriggs Oct 25, 2018

Choose a reason for hiding this comment

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

Yes, that's the intent. Since we're copying a versioned installer into a non-versioned directory, I want to ensure that we don't have multiple. The correctness of the installation during PrepFunctionalTests.sh depends on there being exactly the one dmg we want to install in $VFS_OUTPUTDIR/Git

@jrbriggs jrbriggs changed the title Modify PrepFunctionalTests.sh to work out of the publish directory Modify PrepFunctionalTests.sh to work out of the output directories Oct 26, 2018
@jrbriggs jrbriggs merged commit d428f1a into microsoft:master Oct 26, 2018
@jrbriggs jrbriggs deleted the publish-git branch October 26, 2018 12:15
@jrbriggs jrbriggs added this to the S147 milestone Feb 7, 2019
@jrbriggs jrbriggs added the affects: engineering Keeping the engineering system healthy label Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: engineering Keeping the engineering system healthy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants