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

[Dark Theme] Fix failing functional tests related to minification #9935

Conversation

martinrrm
Copy link

@martinrrm martinrrm commented Apr 24, 2024

Functional tests related to minification started to fail with this new feature.
What is happening is that CSS minification isn't able to understand the CSS variables because we are using an old package to handle bundle/minification (System.Web.Optimization from 2014).

Found:    Minification failed
      In value: /* Minification failed. Returning unminified contents.
      (296,15): run-time error CSS1039: Token not allowed after unary operator: '-brandForegroundLinkRest'
      (301,15): run-time error CSS1039: Token not allowed after unary operator: '-brandForegroundLinkHover'
      (306,27): run-time error CSS1039: Token not allowed after unary operator: '-neutralStrokeFocus2Rest'
      (533,15): run-time error CSS1039: Token not allowed after unary operator: '-neutralForeground3Rest'
      (622,26): run-time error CSS1039: Token not allowed after unary operator: '-neutralBackground5Rest'
      (1582,15): run-time error CSS1039: Token not allowed after unary operator: '-neutralForeground3Rest'
      (1819,15): run-time error CSS1039: Token not allowed after unary operator: '-neutralForeground2Rest'
      (1821,33): run-time error CSS1039: Token not allowed after unary operator: '-neutralStroke2Rest'
      (1838,15): run-time error CSS1039: Token not allowed after unary operator: '-neutralForeground1Rest'
      (1859,26): run-time error CSS1039: Token not allowed af
      Stack Trace:
        D:\a\_work\1\s\tests\NuGetGallery.FunctionalTests\StaticAssets\StaticAssetsTests.cs(145,0): at NuGetGallery.FunctionalTests.StaticAssets.StaticAssetsTests.<NoBundleFailsMinification>d__14.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

Fix

Grunt also creates minified files every time we use grunt command, with this change I'm no longer adding the files to the site.min.css bundle to avoid minification and instead use the minified file we already have.

Addresses #9936

@martinrrm martinrrm requested a review from a team as a code owner April 24, 2024 18:47
@martinrrm martinrrm changed the title Fix functional tests [Dark Theme] Fix failing functional tests related to minification Apr 24, 2024
@@ -118,8 +118,6 @@ private static void BundlingPostStart()
// Add scripts bundles
var newStyleBundle = new StyleBundle("~/Content/gallery/css/site.min.css");
newStyleBundle
.Include("~/Content/gallery/css/bootstrap.css")
Copy link
Author

Choose a reason for hiding this comment

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

Removing these files from the sites min since we already have the minified files

Copy link
Member

Choose a reason for hiding this comment

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

Consider leaving a comment about why boostrap is not included here

@martinrrm
Copy link
Author

Functional tests are failing when I added a new test, not sure why, this is the error. I'm continuing the investigation here.

image

@martinrrm martinrrm force-pushed the dev-martinrrm-feature-new-theme branch from e85c08c to 1b17d73 Compare April 30, 2024 17:55
@martinrrm martinrrm force-pushed the dev-martinrrm-fix-functional-tests-dark-theme branch from b97e8e4 to c750fa9 Compare May 6, 2024 17:06
@martinrrm martinrrm force-pushed the dev-martinrrm-feature-new-theme branch from 1b17d73 to 3ce9603 Compare May 7, 2024 20:06
@erdembayar
Copy link
Contributor

@martinrrm
There's merge conflict needs to be addressed

.pipelines/Release-trigger.yml Outdated Show resolved Hide resolved
@martinrrm martinrrm force-pushed the dev-martinrrm-fix-functional-tests-dark-theme branch 2 times, most recently from 3f6c458 to ddd10df Compare May 16, 2024 17:27
@martinrrm martinrrm force-pushed the dev-martinrrm-feature-new-theme branch from 3ce9603 to 1dba8a0 Compare May 16, 2024 17:30
@martinrrm martinrrm force-pushed the dev-martinrrm-fix-functional-tests-dark-theme branch from ddd10df to 6cbc94d Compare May 16, 2024 17:31
@martinrrm martinrrm force-pushed the dev-martinrrm-fix-functional-tests-dark-theme branch from 9012138 to 133a070 Compare May 17, 2024 19:03
@martinrrm
Copy link
Author

@joelverhagen @agr Finally fixed tests https://dev.azure.com/nuget/NuGetBuild/_build/results?buildId=100646&view=results. Code has changed a little bit but it's the same solution.

When bundling bootstrap.css to styles.min.css it is minifying the file but the tool that does it doesn't understand about CSS variables because is from 2014, and it no longer maintained.

This solution is to add bootstrap.min.css to the source files and use it, we were already creating it when running grunt, so it's not adding a lot of new code and avoid re-minifying it.

We only need to reference it in one file like we are doing with styles.min.css

@erdembayar
Copy link
Contributor

When bundling bootstrap.css to styles.min.css it is minifying the file but the tool that does it doesn't understand about CSS variables because is from 2014, and it no longer maintained.

Which tool are you referring here?

@martinrrm
Copy link
Author

@erdembayar So I've looked in the documentation and found that is using this package https://learn.microsoft.com/en-us/previous-versions/aspnet/hh195125(v=vs.110) which I believe it's been outdated since 2013.

There is some documentation on how to minify other type of files (less, scss and sass which are other preprocessors) here https://learn.microsoft.com/en-us/aspnet/mvc/overview/performance/bundling-and-minification#less-coffeescript-scss-sass-bundling, they recommend installing another package and parsing the file with it. Which technically is what I'm doing here but with grunt.

@martinrrm martinrrm merged commit 92afa45 into dev-martinrrm-feature-new-theme May 20, 2024
2 checks passed
martinrrm added a commit that referenced this pull request May 22, 2024
)

* Fix functional tests related to minified files
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.

None yet

4 participants