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

Fix failing test #112

Conversation

jacobherrington
Copy link

This change fixes one of the two failing tests. It seems like a somewhat undocumented change happened between Loofah 2.8.0 and 2.9.0, causing the output of scrubbed style attributes to look a bit different.

Additionally, it adds two tests to ensure the work being done here doesn't break the feature added in 2.9.0.

It's worth bringing up that there was a regression introduced in Loofah 2.9.0 and it has been patched today. That issue might pop up here as well.

# Commit messages for more context!
dbff002:
Fix failing unit test

One of the changes in released in Loofah 2.9.0 broke this test. I can't
tell if this was an intended or unintended side effect of the change to
support functions in shorthand CSS rule.

I'm not sure how well documented this change was, but Loofah's tests
were updated to reflect a different formatting in the commit that causes
this behavior. I think it's safe to say that stripping the whitespace
from rules like the one in this test is the expected behavior.

The test should be updated to reflect the new formatting.

bca8940:
Add regression test for CSS functions

Loofah 2.9.0 added support for "safe" CSS functions in shorthand CSS
properties, e.g. `background: rgba(255,0,0,0.3);`

Adding a couple of tests to ensure that functionality works in this gem
too makes sense.

One of the changes in released in Loofah 2.9.0 broke this test. I can't
tell if this was an intended or unintended side effect of the change to
support functions in shorthand CSS rule.

I'm not sure how well documented this change was, but Loofah's tests
were updated to reflect a different formatting in the commit that causes
this behavior. I think it's safe to say that stripping the whitespace
from rules like the one in this test is the expected behavior.

The test should be updated to reflect the new formatting.

Relevant links from Loofah:
https://github.com/flavorjones/loofah/releases/tag/v2.9.0
flavorjones/loofah#200
Loofah 2.9.0 added support for "safe" CSS functions in shorthand CSS
properties, e.g. `background: rgba(255,0,0,0.3);`

Adding a couple of tests to ensure that functionality works in this gem
too makes sense.
@flavorjones
Copy link
Member

Apologies, my PR at #113 potentially conflicts with this. I'm happy to rebase my PR, just let me know.

@jacobherrington
Copy link
Author

Superseded by #113 👏

@jacobherrington jacobherrington deleted the fix-failing-test-sanitizer-272 branch April 9, 2021 03:10
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

2 participants