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

Add multipass tests #1177

Closed
wants to merge 15 commits into from
Closed

Add multipass tests #1177

wants to merge 15 commits into from

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Oct 30, 2019

@GreLI:

  • This PR adds tests for multipass in svgo. Currently, there aren't any tests for multipass at all.
  • It also fixes a bug with multipassCount that was uncovered with these newly added multipass tests.
  • A test for the prefixIds plugin with multipass is added.
  • A test for file name handling of the prefixIds plugin is added (it is very similar to the test for prefixIds with multipass, so it seems to be a nice addition).
  • A test for the addAttributesToSVGElement plugin with multipass is added.
  • Adds a test (albeit retroactively) for invoking svgo.optimize() without passing it an info object.

@GreLI: Additional points:

Edit: What about using a separate tests folder for the plugin tests with multipass enabled?

@dphil
Copy link

dphil commented Jan 3, 2020

With this approach, the first pass has multipassCount as undefined, rather than 0. With the way multipassCount is currently used (the check in prefixIds), it makes no difference, but initializing it to 0 would likely be more appropriate and robust.

@strarsis
Copy link
Contributor Author

strarsis commented Jan 4, 2020

@dphil: multipassCounter is now initialized to 0 at the start: https://github.com/svg/svgo/pull/1177/files#diff-2338951073175db78c5b00c29d4971e1R26

I added the undefined info object test from the other PR to this PR because it fits the whole issue well.

@strarsis
Copy link
Contributor Author

strarsis commented Jan 4, 2020

@GreLI: The build of this PR currently fails for node v6.
This is not caused by this PR but is an actual issue with the current svgo master.
PR for current master to fix failing build on node v6: #1208

@strarsis
Copy link
Contributor Author

strarsis commented Mar 9, 2020

@kylemh: I got an email notification from GitHub with a comment from you about the prefixIds plugin not working correctly with multipass and specific options. I added a test for this set of options and the test passes. Are you using a specific node version? There were node-specific issues in the past.

@kylemh
Copy link

kylemh commented Mar 9, 2020

I was hasty and incorrect. It had to do with the difference of build-time implementation in one file versus the run-time CLI usage again source files in their individual locations.

@strarsis
Copy link
Contributor Author

strarsis commented Mar 9, 2020

@kylemh: Well, it never hurts to have some more tests.

@kylemh
Copy link

kylemh commented Mar 9, 2020

I’ll appreciate it in my fork when I steal it 😂

strarsis and others added 14 commits February 14, 2021 19:44
Fix multipassCounter code to make failed multipass tests pass.
Improve multipass related tests.
cleanupIDs plugin is disabled when a script or style tag is present. I
assume that this is because they might contain an id.

If these tags are empty there is no point in not cleaning the id though.
When running svgo with the option `multipass: true`, it allows us to
first run `inlineStyles` plugin to empty the style tag. Then, we can run
`cleanupIDs` plugin.
@TrySound
Copy link
Member

Hi @strarsis Could you please rebase?

@kylemh
Copy link

kylemh commented Feb 18, 2021

Glad you got added to the team @TrySound

TrySound added a commit that referenced this pull request Feb 18, 2021
Ref
  #1330
  #1148
  #1133
  #1227
Took tests from #1177
@TrySound TrySound mentioned this pull request Feb 18, 2021
TrySound added a commit that referenced this pull request Feb 18, 2021
@TrySound TrySound closed this Feb 18, 2021
@svg svg deleted a comment from MrSweGyii Feb 18, 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