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 failed specs in Crystal v1.13 #1909

Merged
merged 4 commits into from
Aug 12, 2024

Conversation

akadusei
Copy link
Contributor

Purpose

Fix failed Lucky::CookieJar specs in Crystal v1.13

Description

Lucky::CookieJar specs fail in Crystal v1.13. This is related to crystal-lang/crystal#14455. This PR fixes that.

See https://github.com/luckyframework/lucky/actions/runs/10340714956/job/28621689266.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

```
Failures:

  1) Lucky::CookieJar raises a nicer error for invalid cookie values
     Failure/Error: expect_raises(Lucky::InvalidCookieValueError, "Cookie value for 'cookie' is invalid") do

       Expected Lucky::InvalidCookieValueError but nothing was raised

Error:      # spec/lucky/cookies/cookie_jar_spec.cr:41
```

Crystal v1.13 accepts space in cookie values as valid.

See <crystal-lang/crystal#14455>.
@akadusei
Copy link
Contributor Author

There's also crystal-loot/exception_page#49, which breaks Lucky when using shard.edge.yml.

@jwoertink
Copy link
Member

Is this the right code? It looks like #1908 ? 🤔

@jwoertink
Copy link
Member

oohh... I see where the change was made. Ok. Yeah, looks like the other one snuck in here 😂

@@ -35,7 +35,7 @@ describe Lucky::CookieJar do
end

it "raises a nicer error for invalid cookie values" do
value = "Double Chocolate"
value = "Double,Chocolate"
Copy link
Member

Choose a reason for hiding this comment

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

👍

@akadusei
Copy link
Contributor Author

Oops, sorry! I forgot to checkout main before working on this

@akadusei
Copy link
Contributor Author

I can close this and send a new PR if that is preferred?

@jwoertink
Copy link
Member

It's ok. I was going to merge the other anyway, so we'll just lump them together :D

@jwoertink jwoertink merged commit 12d55b6 into luckyframework:main Aug 12, 2024
4 of 5 checks passed
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.

2 participants