Skip to content

FunctionalTests - Clean up publish of deps.json #29449

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

Merged
merged 7 commits into from
Jan 25, 2021
Merged

FunctionalTests - Clean up publish of deps.json #29449

merged 7 commits into from
Jan 25, 2021

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jan 20, 2021

Part of #27296

  • Copy deps.json as part of build task
  • Remove dead code

@Pilchie Pilchie added area-identity Includes: Identity and providers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jan 20, 2021
@HaoK HaoK changed the title Clean up publish of deps.json FunctionalTests - Clean up publish of deps.json Jan 20, 2021
@HaoK HaoK marked this pull request as ready for review January 21, 2021 04:37
@HaoK HaoK requested a review from a team January 21, 2021 04:37
<WriteLinesToFile File="$(PublishDir)\contentroot.sln" Lines="Ignored" Overwrite="true" Encoding="Unicode" />
<Target Name="PublishAssets" AfterTargets="Publish">
<ItemGroup>
<_PublishFiles Include="$(ArtifactsBinDir)Microsoft.AspNetCore.Identity.UI\$(Configuration)\$(DefaultNetCoreTargetFramework)\Microsoft.AspNetCore.Identity.UI.Views.*.dll" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the UI functional tests need some additional dll/pages, in addition to the deps json

Copy link
Member

Choose a reason for hiding this comment

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

Yep, these switch the application parts for testing V3 and V4 bootstrap I believe. We might be able to take them out of here since we removed bootstrap 3 support I believe, but we'll have to redo this work every time we have two ongoing supported versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

So these are fine to just leave for now in the csproj? The tests fail to run without these files

Copy link
Member

Choose a reason for hiding this comment

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

@HaoK I see what you did now. It's glorious!!

</ItemGroup>
<Copy SourceFiles="@(_PublishFiles)" DestinationFolder="$(PublishDir)" />
</Target>

Copy link
Member

Choose a reason for hiding this comment

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

What was the magic behind removing this, we simply don't need it anymore? (We used to need it for application parts and Razor runtime compilation and we use the API within the WAF for something but can't remember where/why)

Copy link
Member Author

Choose a reason for hiding this comment

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

These weren't here in the MVC Functionals csproj until my last PR, I just added them to copy over the deps file to get them to the publish directory for helix to pickup, that copy logic is now just in the build task, so this is just reverting the hack from the last PR

Copy link
Member Author

Choose a reason for hiding this comment

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

That said, can you think of any reasons why script and link tags would change their rendering output now that they are running in the publish directory instead of the src directory? Is it something to do with how relative url/paths might render, and I just need to update the baselines?

Copy link
Member

Choose a reason for hiding this comment

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

Release/Debug + Environment tag helper

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I will follow up in a 3rd PR just looking at restoring those 2-3 skipped tests since this one is basically ready to go

{
// When publishing content root is always the BaseDirectory
output[assemblyName] = "~";
var depsFile = project.GetMetadata("DepsFile");
Copy link
Member

Choose a reason for hiding this comment

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

💯 This is glorious!

Log.LogMessage("Looking for " + depsFile + ": "+ File.Exists(depsFile));
if (File.Exists(depsFile))
{
File.Copy(depsFile, Path.Combine(PathToCopyDeps, Path.GetFileName(depsFile)));
Copy link
Member

Choose a reason for hiding this comment

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

One small nit is that it might be better to do this within a target so that we can leverage <Copy which will do retries and so on.

I think it might be easier to just add these files to the resolvedfilestopublish itemgroup and that way it reuses the same standard insfrastructure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, yeah that looks better than doing it in the task, updated with doing it in the targets file

Base automatically changed from master to main January 22, 2021 01:33
@HaoK
Copy link
Member Author

HaoK commented Jan 25, 2021

@javiercn look good now for merge?

@HaoK HaoK merged commit 7244d55 into main Jan 25, 2021
@HaoK HaoK deleted the haok/pub branch January 25, 2021 17:16
@HaoK HaoK mentioned this pull request Jan 25, 2021
5 tasks
@HaoK HaoK added this to the 6.0-preview1 milestone Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants