Cassandane::Tiny: Import strict/warnings to consumers #5117
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(This identified a bunch of compilation problems that need to be cleaned up; I'll work on that and include them in this MR...) |
wolfsage
force-pushed
the
cassandane-tiny-strict-warnings
branch
from
e11c352
to
c756170
Compare
This way any tiny test gets them by default, avoiding mistakes like forgetting to declare variables, etc...
- Declare variables where we forgot to
- Don't declare the same variable multiple times
- Things like {alerts\n} are treated as a function lookup, so wrap the keys
in quotes (or reflow them to be on one line)
- sort wants $a and $b, not $_ (which was a random string!)
wolfsage
force-pushed
the
cassandane-tiny-strict-warnings
branch
from
c756170
to
bbc7429
Compare
tiny-tests/Caldav/options was throwing: Variable "$caldav" will not stay shared at ./tiny-tests/Caldav/options line 11. This is because it was defining a subroutine that was closing over variables inside of another subroutine. This generally doesn't do what you expect, so instead lets make the subroutine itself an anonymous subroutine which will be generated at runtime and close over outer scope variables properly. In newer Perl we could use a 'my sub...', but I don't believe we are aiming to require a high enough version of Perl yet.
This test was doing `foo => qw(3,7)`, and then later attempting to assign
that to a list of variables. But this doesn't work right for three reasons:
1. foo => qw(3,7) - qw() makes a list of items delimted by spaces, not
commas, so this was returning a single item, the string
"3,7".
2. If this was `foo => qw(3 7)` as likely intended, it would assign the
value `3` to the key `foo`, and then create a key `7` with the
value `undef`!
3. The assignment `my ($needMaj, $needMin) = $tc->{skipVersionBefore}` does
not dereference the value of skipVersionBefore, so it only ever sets
`$needMaj`.
So let's switch to using arrayrefs directly and dereference them when
needed.
Before we didn't have strict, do dereferencing undef was fine. With strict we have to be more careful/explicit.
And $_ would have contained a non-ref, and so crashed!
Without strict it didn't matter, but with strict it does.
Back in d7c8b4f the uid property was removed from default alerts. This test wasn't updated to account for that, but continued to pass because my $uid = (values %{$defaultAlertsWithTime})->{uid}; $self->assert_matches(qr/UID:$uid/, $res->{content}); would end up with an undef value in $uid and so the assertion would succeed. Now with strict we see that (values %{$defaultAlertsWithTime})->{uid} crashes because values uses in this way returns a count of how many items were in the hash, and so we try to deref that as a hashref and fail. Instead, ensure that depending on which alarms are currently configured, we get the correct alarm id for both the owner and the sharee.
This is something we need to solve more generally, but the main problem is that each tiny test actually exists in the parent package that has loaded it, so any subroutines it defines is visible in the parent. This means if two tests define the same sub, one of those subs will be clobbered. Ugh. For now, let's side step the problem by giving the two (identical) subroutines different names. This is a hack, but puts us in a better place, and we can solve the real problem later.
|
This should be all set |
|
Waiting for @elliefm to chime in, too. |
|
Wow, thanks. |
elliefm
approved these changes
Nov 7, 2024
| } | ||
|
|
||
| sub _can_match { | ||
| sub _can_match_b { |
There was a problem hiding this comment.
Are these two functions identical? In that case I would've just moved it up to the parent. The fact that you gave them distinct names instead makes me think maybe they were distinct functions sharing a name, but the bodies don't show in the diff and I didn't look any deeper to find out
ksmurchison
approved these changes
Nov 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This way any tiny test gets them by default, avoiding mistakes like forgetting to declare variables, etc...