[
https://issues.apache.org/jira/browse/CASSANDRA-17009?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17423035#comment-17423035
]
Brian Houser commented on CASSANDRA-17009:
------------------------------------------
> I noted a few minor typos in the PR
I accidentally turned off the spell checker in the ide as it turns out... blerg
I'll fix them, sorry about that.
> Sthg weird happened with the file {{cassandra.in.sh}}. Make sure you're
> branching off the right place etc
not sure how that ended up in the mix and it has no changes, I'll remove it. I
was branching off cassandra-4.0 at the time of submitting
> I would use this opportunity to add {{assertOnCleanExit()}} on the already
> existing test when possible
Np, I'll go ahead and do that.
>That bug on the return code I would fix in this ticket. Bear in mind CI is
>heavy and we'll need to ping more people to review this ticket. The less we go
>back and forth the better. So if we can pack it here, within reason, I'd do
>that. Seems to me It'd be a few loc only.
The fix for this is small, but I was trying to keep the change small,
incremental and focused on the unit test. No sweat though, I can fix it
(basically just force an early exit).
> The issue with the git ignore of {{test/data}} sounds weird to me it didn't
> came up sooner. I have to give it another thought.
Yeah it was a weird thing to me when I found it, pretty anti-intuitive and I
had to look up the git docs for fear I was being nutty. I mentioned it on the
slack channel before doing this. We have are ignoring "data/" which ignores
any folder containing data. It hasn't come up because any time that I guess we
added to it we just used the force option (also older files will avoid the
gitignore by virtue of being grandfathered in). It seemed like a good idea to
file an exception, and chatting with folk they said I should just jump on it.
From [https://git-scm.com/docs/gitignore] * If there is a separator at the end
of the pattern then the pattern will only match directories, otherwise the
pattern can match both files and directories.
* For example, a pattern {{doc/frotz/}} matches {{doc/frotz}} directory, but
not {{a/doc/frotz}} directory; however {{frotz/}} matches {{frotz}} and
{{a/frotz}} that is a directory (all paths are relative from the {{.gitignore}}
file).
I think this is the {{frotz/}} case here.Just to make sure I wasn’t crazy, I
created a file inside of test/data to see if was being tracked, my git ignored
it.
I called this “helloworld”. added some text and then ran…
$ git check-ignore -v helloworld
.gitignore:8:data/ helloworld
>The already existing tests only 'scratch the surface'. They pass an argument
>and check the tool doesn't explode. But they don't check the sstables
>themselves to make sure the argument was effective. I.e does '-r' really have
>an effect?
For the most part, this is a verifier, so its job is to tell you if something
is messed up, not necessarily do much about it(or at least that was my
thought). The tests verify you get a verify result. Though I could see about
adding a `-r` as that has the potential to mutate.
There is already a verifyer test (which is mainly all this class exercises)
called VerifyTest, this test already covers said cases. I thought it would be
somewhat redundant to replicate most of the same tests in a unit test. I
thought better to make sure that the happy and unhappy verifyer results are
caught and verify the execution paths.
> The new tests you added are really nice! but for completion purposes I would
> combine them with -q, -e, etc and any other params to make sure they all work
> with corrupted data i.e.
> We seem to be missing a -c test.
I can add these, though as I said, at that point I am mostly doing similar
tests as in VerifyTest (they really unit test that component through the
verifier). It won't add to the coverage of the suite.
> Sstableverify unit test operate on SSTables
> -------------------------------------------
>
> Key: CASSANDRA-17009
> URL: https://issues.apache.org/jira/browse/CASSANDRA-17009
> Project: Cassandra
> Issue Type: Sub-task
> Components: Tool/sstable
> Reporter: Brian Houser
> Assignee: Brian Houser
> Priority: Normal
> Time Spent: 2h
> Remaining Estimate: 0h
>
> as part of https://issues.apache.org/jira/browse/CASSANDRA-16009, unit
> coverage is a bit lax and doesn't run through the verifier (based on my
> coverage results).
> There should be a unit test that exercises the internal verifier both for a
> corrupt example and a working example.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
