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

test: fix misc. test warnings #1161

Merged
merged 4 commits into from Jul 8, 2021
Merged

test: fix misc. test warnings #1161

merged 4 commits into from Jul 8, 2021

Conversation

sigveio
Copy link
Collaborator

@sigveio sigveio commented Jul 7, 2021

Summary

This pull request aims to address some minor issues leading to warnings during test running.

A sample of the 3 different types of warnings encountered:

Screenshot 2021-07-07 at 21 29 38
Screenshot 2021-07-07 at 21 29 09
Screenshot 2021-07-07 at 21 28 48

It also adds ignores for two test helper files which had ignore patterns for test running but not coverage reporting.

Are you introducing a breaking change?

  • Yes
  • No (only touches tests - in the form of refactoring)

- Replace deprecated postcss.plugin syntax
- Add { from: undefined } option to prevent warnings
- Dummy workaround for calling postcss w/o plugins
@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2021

Codecov Report

Merging #1161 (2c9e251) into master (629918c) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1161      +/-   ##
==========================================
+ Coverage   96.43%   96.53%   +0.10%     
==========================================
  Files         116      114       -2     
  Lines        3592     3582      -10     
  Branches     1054     1052       -2     
==========================================
- Hits         3464     3458       -6     
+ Misses        119      115       -4     
  Partials        9        9              
Impacted Files Coverage Δ
packages/cssnano/src/__tests__/_processCss.js
packages/cssnano/src/__tests__/_webpack.config.js

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 629918c...2c9e251. Read the comment docs.

packages/cssnano/src/__tests__/issue927.js Show resolved Hide resolved
@@ -1,8 +1,15 @@
import postcss from 'postcss';
import sameParent from '../sameParent';

const plugin = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there another way to suppress the warnings? In fact we want postcss to do nothing here. I think adding more code to the test does not help with clarity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well... my rationale was that when I opened up that file, not knowing the code at all, it was not clear to me at first that you did in fact not want postcss to do anything. So although the dummy plugin at the top of the file does add a little bit of code - could it not also help point out the intention by explicitly passing in a "dummy" instead of nothing?

I didn't see any obvious alternative way (e.g. an option for postcss), but admittedly I also didn't look super hard. Would be happy to have another look though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think just adding a comment to explain what's going on is the best solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright - I'll look into it a bit more thoroughly and see if I find a better solution to suppress the warnings then :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, so there is actually an option to suppress it:

https://github.com/postcss/postcss/blob/10d974f81d2b925a9e8c0912857dcda1bf6720c0/lib/processor.js#L19

So;

test('should calculate same parent', () => {
  return postcss()
    .process('h1 {} h2 {}', { from: undefined, hideNothingWarning: true })
    .then((result) => {
      (...)
    });
});

Works!

jest.config.js Show resolved Hide resolved
@sigveio sigveio marked this pull request as draft July 8, 2021 11:08
@sigveio sigveio marked this pull request as ready for review July 8, 2021 12:18
@@ -3,7 +3,7 @@ import sameParent from '../sameParent';

test('should calculate same parent', () => {
return postcss()
.process('h1 {} h2 {}', { from: undefined })
.process('h1 {} h2 {}', { from: undefined, hideNothingWarning: true })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow! You've managed to find a hidden postcss option!

@ludofischer ludofischer merged commit 095f901 into cssnano:master Jul 8, 2021
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

3 participants