Re: Some tests are never executed in CI due to their name

[Josh McKenzie]

To Derek's point, why not go with:

> Would it be sufficient to look for files that don't end in "Test.java" but 
> contain "@Test" annotations?
+ exclude files that contain "abstract class" in them that matches the file 
name?

Combine that w/checking specific subdirectories and we relax the required 
naming convention quite a bit; makes it easy to do the right thing and hard to 
do the wrong thing.

It's also very simple which is a feature.

On Fri, Oct 28, 2022, at 4:17 AM, Miklosovic, Stefan wrote:
> Hi Berenguer,
> 
> I agree. We can always go heavy-weight anyway. I will dedicate more time to 
> that the next week. It would be nice if you tagged along and help with the 
> review.
> 
> FYI the ticket is touching all branches we support (there are some skipped 
> tests in 3.0 too), we would just add checkstyle to 4.1 and trunk as 
> discussed, so I expect we fix all 5 branches at once.
> 
> Regards
> 
> Stefan
> 
> 
> From: Berenguer Blasi 
> Sent: Friday, October 28, 2022 9:43
> To: dev@cassandra.apache.org
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> 
> Hi Stefan,
> 
> seems none of the 2 proposals are strongly opposed to. If you feel more 
> comfortable with option 1 I say we do that, which will always be a step in 
> the right direction. If we see it bites us again in the future then we do 2. 
> But at least we bring this benefit/ticket in. I would also be happy to drive 
> option 2 to completion if needed.
> 
> wdyt?
> 
> On 25/10/22 14:35, Berenguer Blasi wrote:
> 
> Well examples lend themselves to many things. I am sure I could come up with 
> examples in the opposite direction. I'd rather talk in terms of the actual 
> files and tests we have.
> 
> What I was trying to clarify is what I was misunderstanding as I had the 
> impression now we were saying option 1 would not leak again.
> 
> So option 2, given ant and CI seem to rely on the *Test.java notation and 
> also prevents future leaks doesn't seem to me a bad idea. Renaming the 
> offending files seems a reasonable cost to me (Eclipse does it for me).
> 
> My 2cts! But hey, I've been known to be wrong before :shrug: lol
> 
> On 25/10/22 12:04, Andrés de la Peña wrote:
> Note that the test multiplexer also searches for tests ending with "Test", so 
> it will also miss new or modified tests with nonstandard names. The 
> automatically detected tests are listed when running generate.sh, and they 
> are added to the repeated run jobs. That gives us another opportunity to 
> detect missed tests when we introduce something like "MyTestSplit1.java" and 
> see that it's not been included in the mandatory repeated runs.
> 
> On Tue, 25 Oct 2022 at 10:31, Miklosovic, Stefan 
> mailto:stefan.mikloso...@netapp.com>> wrote:
> Hi Berenguer,
> 
> I am glad you asked. I was expecting this question.
> 
> I think there is a fundamental difference in how we approach this problem. I 
> do not say mine is better, I just find it important to describe it clearly.
> 
> Let's say we are on a ship which leaks. When I detect that, my course of 
> action is to fix the leakage with the biggest patches I can find around with 
> minimal effort necessary so we are not taking water anymore. There might 
> still be occasional leakages which are very rare but I have a crew who is 
> checking the ship constantly anyway (review) and the risk that big leakages 
> like we just fixed happen again are relatively very low.
> 
> You spot a leakage and instead of fixing it with big patches and calling it a 
> day, you are trying to remodel the cabins so they are completely waterproof 
> but while doing so, you renamed them so everyone needs to get used to how 
> these cabins are called and where they are located, because there is this 
> minimal chance that some cadet comes around and starts to live in a cabin he 
> is not supposed to and we need to explain it to him. Thousands of cadets 
> found their cabins just fine. Occasionally, there are few people who just 
> miss the right board completely (Test at start, Tests at the end) and they 
> are automatically navigated around (checkstyle) but once in five years there 
> is this guy who just completely missed it.
> 
> I believe we can just navigate him when that happens. You want to cover that 
> guy too.
> 
> I just find my approach easier.
> 
> You can remodel it all, for sure but I am afraid I will not be a part of 
> that. I just do not find it necessary to do that.
> 
> 
> From: Berenguer Blasi 
> mailto:berenguerbl...@gmail.com>>
> Sent: Tuesday, October 25, 2022 11:08
> To: dev@cassandra.apache.org
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. 

Re: Some tests are never executed in CI due to their name

[Miklosovic, Stefan]

I do not think there is any module in checkstyle which would do what you want.


From: Josh McKenzie 
Sent: Monday, October 31, 2022 12:49
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



To Derek's point, why not go with:

Would it be sufficient to look for files that don't end in "Test.java" but 
contain "@Test" annotations?
+ exclude files that contain "abstract class" in them that matches the file 
name?

Combine that w/checking specific subdirectories and we relax the required 
naming convention quite a bit; makes it easy to do the right thing and hard to 
do the wrong thing.

It's also very simple which is a feature.

On Fri, Oct 28, 2022, at 4:17 AM, Miklosovic, Stefan wrote:
Hi Berenguer,

I agree. We can always go heavy-weight anyway. I will dedicate more time to 
that the next week. It would be nice if you tagged along and help with the 
review.

FYI the ticket is touching all branches we support (there are some skipped 
tests in 3.0 too), we would just add checkstyle to 4.1 and trunk as discussed, 
so I expect we fix all 5 branches at once.

Regards

Stefan


From: Berenguer Blasi 
mailto:berenguerbl...@gmail.com>>
Sent: Friday, October 28, 2022 9:43
To: dev@cassandra.apache.org
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.




Hi Stefan,

seems none of the 2 proposals are strongly opposed to. If you feel more 
comfortable with option 1 I say we do that, which will always be a step in the 
right direction. If we see it bites us again in the future then we do 2. But at 
least we bring this benefit/ticket in. I would also be happy to drive option 2 
to completion if needed.

wdyt?

On 25/10/22 14:35, Berenguer Blasi wrote:

Well examples lend themselves to many things. I am sure I could come up with 
examples in the opposite direction. I'd rather talk in terms of the actual 
files and tests we have.

What I was trying to clarify is what I was misunderstanding as I had the 
impression now we were saying option 1 would not leak again.

So option 2, given ant and CI seem to rely on the *Test.java notation and also 
prevents future leaks doesn't seem to me a bad idea. Renaming the offending 
files seems a reasonable cost to me (Eclipse does it for me).

My 2cts! But hey, I've been known to be wrong before :shrug: lol

On 25/10/22 12:04, Andrés de la Peña wrote:
Note that the test multiplexer also searches for tests ending with "Test", so 
it will also miss new or modified tests with nonstandard names. The 
automatically detected tests are listed when running generate.sh, and they are 
added to the repeated run jobs. That gives us another opportunity to detect 
missed tests when we introduce something like "MyTestSplit1.java" and see that 
it's not been included in the mandatory repeated runs.

On Tue, 25 Oct 2022 at 10:31, Miklosovic, Stefan 
mailto:stefan.mikloso...@netapp.com>>>
 wrote:
Hi Berenguer,

I am glad you asked. I was expecting this question.

I think there is a fundamental difference in how we approach this problem. I do 
not say mine is better, I just find it important to describe it clearly.

Let's say we are on a ship which leaks. When I detect that, my course of action 
is to fix the leakage with the biggest patches I can find around with minimal 
effort necessary so we are not taking water anymore. There might still be 
occasional leakages which are very rare but I have a crew who is checking the 
ship constantly anyway (review) and the risk that big leakages like we just 
fixed happen again are relatively very low.

You spot a leakage and instead of fixing it with big patches and calling it a 
day, you are trying to remodel the cabins so they are completely waterproof but 
while doing so, you renamed them so everyone needs to get used to how these 
cabins are called and where they are located, because there is this minimal 
chance that some cadet comes around and starts to live in a cabin he is not 
supposed to and we need to explain it to him. Thousands of cadets found their 
cabins just fine. Occasionally, there are few people who just miss the right 
board completely (Test at start, Tests at the end) and they are automatically 
navigated around (checkstyle) but once in five years there is this guy who just 
completely missed it.

I believe we can just navigate him when that happens. You want to cover that 
guy too.

I just find my approach easier.

You can remodel it all, for sure but I am afraid I will not be a part of that. 
I just do not fin

Re: Some tests are never executed in CI due to their name

[Miklosovic, Stefan]

I always hit "send" before realizing I have other points to add :)

I was thinking we want to go with checkstyle and check the name. Not manually 
parsing the file names. Do not we also want these test files to look 
consistent? I do not think that having a test class called "SdsdakASsdadsada" 
which happen to contain @Test and is not abstract should be considered to be a 
valid file name / test class. Do not we also want to enforce some regularity 
into this?

Another point is that even we parsed we so it will be picked up while tests are 
executed, that means that it will not be checked on the commit?

Lastly, how do you actually want to do this if we currently have test.name 
property which is a simple regexp (*Test)? -Dtest.name is pretty much how we do 
this currently and I am not sure we want to break this in all CI scripts.

I do not think we are actually winning anything with that approach.

Regards


From: Josh McKenzie 
Sent: Monday, October 31, 2022 12:49
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



To Derek's point, why not go with:

Would it be sufficient to look for files that don't end in "Test.java" but 
contain "@Test" annotations?
+ exclude files that contain "abstract class" in them that matches the file 
name?

Combine that w/checking specific subdirectories and we relax the required 
naming convention quite a bit; makes it easy to do the right thing and hard to 
do the wrong thing.

It's also very simple which is a feature.

On Fri, Oct 28, 2022, at 4:17 AM, Miklosovic, Stefan wrote:
Hi Berenguer,

I agree. We can always go heavy-weight anyway. I will dedicate more time to 
that the next week. It would be nice if you tagged along and help with the 
review.

FYI the ticket is touching all branches we support (there are some skipped 
tests in 3.0 too), we would just add checkstyle to 4.1 and trunk as discussed, 
so I expect we fix all 5 branches at once.

Regards

Stefan


From: Berenguer Blasi 
mailto:berenguerbl...@gmail.com>>
Sent: Friday, October 28, 2022 9:43
To: dev@cassandra.apache.org
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.




Hi Stefan,

seems none of the 2 proposals are strongly opposed to. If you feel more 
comfortable with option 1 I say we do that, which will always be a step in the 
right direction. If we see it bites us again in the future then we do 2. But at 
least we bring this benefit/ticket in. I would also be happy to drive option 2 
to completion if needed.

wdyt?

On 25/10/22 14:35, Berenguer Blasi wrote:

Well examples lend themselves to many things. I am sure I could come up with 
examples in the opposite direction. I'd rather talk in terms of the actual 
files and tests we have.

What I was trying to clarify is what I was misunderstanding as I had the 
impression now we were saying option 1 would not leak again.

So option 2, given ant and CI seem to rely on the *Test.java notation and also 
prevents future leaks doesn't seem to me a bad idea. Renaming the offending 
files seems a reasonable cost to me (Eclipse does it for me).

My 2cts! But hey, I've been known to be wrong before :shrug: lol

On 25/10/22 12:04, Andrés de la Peña wrote:
Note that the test multiplexer also searches for tests ending with "Test", so 
it will also miss new or modified tests with nonstandard names. The 
automatically detected tests are listed when running generate.sh, and they are 
added to the repeated run jobs. That gives us another opportunity to detect 
missed tests when we introduce something like "MyTestSplit1.java" and see that 
it's not been included in the mandatory repeated runs.

On Tue, 25 Oct 2022 at 10:31, Miklosovic, Stefan 
mailto:stefan.mikloso...@netapp.com>>>
 wrote:
Hi Berenguer,

I am glad you asked. I was expecting this question.

I think there is a fundamental difference in how we approach this problem. I do 
not say mine is better, I just find it important to describe it clearly.

Let's say we are on a ship which leaks. When I detect that, my course of action 
is to fix the leakage with the biggest patches I can find around with minimal 
effort necessary so we are not taking water anymore. There might still be 
occasional leakages which are very rare but I have a crew who is checking the 
ship constantly anyway (review) and the risk that big leakages like we just 
fixed happen again are relatively very low.

You spot a leakage and instead of fixing it with big patches and calling it a 
day, you are trying to remod

[Marketing] For Review: Changelog blog #20 - October

[Chris Thornett]

Changelog #20, the overview blog for October is open for 72-hr community
review.
Please leave comments and amendments using the comment function.
https://docs.google.com/document/d/1FKX3N5NQ2KAPKwljEiWYDG-A7w5Z70EyFNc6IVYmqms/edit?usp=sharing

Thanks,
-- 

Chris Thornett
Senior Content Strategist, Constantia.io
ch...@constantia.io
Cell: +44 7436-698-860 (Note: UK)
LinkedIn  | Twitter

Re: Some tests are never executed in CI due to their name

[Josh McKenzie]

> I do not think that having a test class called "SdsdakASsdadsada" which 
> happen to contain @Test and is not abstract should be considered to be a 
> valid file name / test class
I mean, I wouldn't recommend it certainly, but I'm not concerned about whether 
a file is "TestThing" or "ThingTest" or "WeTestThing". The ordering of names on 
those files adds very little IMO.

> I do not think we are actually winning anything with that approach.
Well, the name of this thread is "Some tests are never executed in CI due to 
their name" and it fixes that in a robust way without asking of any behavior 
changes of the project community to adhere to an arbitrary rigid naming 
guideline for something. So there's that. :)

On Mon, Oct 31, 2022, at 8:02 AM, Miklosovic, Stefan wrote:
> I always hit "send" before realizing I have other points to add :)
> 
> I was thinking we want to go with checkstyle and check the name. Not manually 
> parsing the file names. Do not we also want these test files to look 
> consistent? I do not think that having a test class called "SdsdakASsdadsada" 
> which happen to contain @Test and is not abstract should be considered to be 
> a valid file name / test class. Do not we also want to enforce some 
> regularity into this?
> 
> Another point is that even we parsed we so it will be picked up while tests 
> are executed, that means that it will not be checked on the commit?
> 
> Lastly, how do you actually want to do this if we currently have test.name 
> property which is a simple regexp (*Test)? -Dtest.name is pretty much how we 
> do this currently and I am not sure we want to break this in all CI scripts.
> 
> I do not think we are actually winning anything with that approach.
> 
> Regards
> 
> 
> From: Josh McKenzie 
> Sent: Monday, October 31, 2022 12:49
> To: dev
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> To Derek's point, why not go with:
> 
> Would it be sufficient to look for files that don't end in "Test.java" but 
> contain "@Test" annotations?
> + exclude files that contain "abstract class" in them that matches the file 
> name?
> 
> Combine that w/checking specific subdirectories and we relax the required 
> naming convention quite a bit; makes it easy to do the right thing and hard 
> to do the wrong thing.
> 
> It's also very simple which is a feature.
> 
> On Fri, Oct 28, 2022, at 4:17 AM, Miklosovic, Stefan wrote:
> Hi Berenguer,
> 
> I agree. We can always go heavy-weight anyway. I will dedicate more time to 
> that the next week. It would be nice if you tagged along and help with the 
> review.
> 
> FYI the ticket is touching all branches we support (there are some skipped 
> tests in 3.0 too), we would just add checkstyle to 4.1 and trunk as 
> discussed, so I expect we fix all 5 branches at once.
> 
> Regards
> 
> Stefan
> 
> 
> From: Berenguer Blasi 
> mailto:berenguerbl...@gmail.com>>
> Sent: Friday, October 28, 2022 9:43
> To: dev@cassandra.apache.org
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> 
> Hi Stefan,
> 
> seems none of the 2 proposals are strongly opposed to. If you feel more 
> comfortable with option 1 I say we do that, which will always be a step in 
> the right direction. If we see it bites us again in the future then we do 2. 
> But at least we bring this benefit/ticket in. I would also be happy to drive 
> option 2 to completion if needed.
> 
> wdyt?
> 
> On 25/10/22 14:35, Berenguer Blasi wrote:
> 
> Well examples lend themselves to many things. I am sure I could come up with 
> examples in the opposite direction. I'd rather talk in terms of the actual 
> files and tests we have.
> 
> What I was trying to clarify is what I was misunderstanding as I had the 
> impression now we were saying option 1 would not leak again.
> 
> So option 2, given ant and CI seem to rely on the *Test.java notation and 
> also prevents future leaks doesn't seem to me a bad idea. Renaming the 
> offending files seems a reasonable cost to me (Eclipse does it for me).
> 
> My 2cts! But hey, I've been known to be wrong before :shrug: lol
> 
> On 25/10/22 12:04, Andrés de la Peña wrote:
> Note that the test multiplexer also searches for tests ending with "Test", so 
> it will also miss new or modified tests with nonstandard names. The 
> automatically detected tests are listed when running generate.sh, and they 
> are added to the repeated run jobs. That gives us another opportunity to 
> detect missed tests when we introduce something like "MyTestSplit1.java" and 
> see that i

Re: Some tests are never executed in CI due to their name

[Miklosovic, Stefan]

I think we are already adhering to them without any effort. Overwhelming 
majority of devs are naming their tests "just right" and I do not think they 
are doing that with any significant effort, probably none. We are only 
re-enforcing the unspoken everybody implicitly agreed on. If somebody does not 
name their tests like the rest, I think it would raise the red flags already 
during a review.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.

I think the crucial question we should ask is how to actually be sure that a 
developer knows that his test not only passed but it was selected to be run 
(now based on *Test). How did these tests which skipped as of now end up there 
in the first place? Somebody just committed them, run the CI and "it was green" 
so they proceed with the commit. But they did not check it was actually run 
because "why not?".

So, we are putting a kind of detector in place to signal that a test is not 
going to be actually even run when the checkstyle fails.


From: Josh McKenzie 
Sent: Monday, October 31, 2022 17:16
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



I do not think that having a test class called "SdsdakASsdadsada" which happen 
to contain @Test and is not abstract should be considered to be a valid file 
name / test class
I mean, I wouldn't recommend it certainly, but I'm not concerned about whether 
a file is "TestThing" or "ThingTest" or "WeTestThing". The ordering of names on 
those files adds very little IMO.

I do not think we are actually winning anything with that approach.
Well, the name of this thread is "Some tests are never executed in CI due to 
their name" and it fixes that in a robust way without asking of any behavior 
changes of the project community to adhere to an arbitrary rigid naming 
guideline for something. So there's that. :)

On Mon, Oct 31, 2022, at 8:02 AM, Miklosovic, Stefan wrote:
I always hit "send" before realizing I have other points to add :)

I was thinking we want to go with checkstyle and check the name. Not manually 
parsing the file names. Do not we also want these test files to look 
consistent? I do not think that having a test class called "SdsdakASsdadsada" 
which happen to contain @Test and is not abstract should be considered to be a 
valid file name / test class. Do not we also want to enforce some regularity 
into this?

Another point is that even we parsed we so it will be picked up while tests are 
executed, that means that it will not be checked on the commit?

Lastly, how do you actually want to do this if we currently have test.name 
property which is a simple regexp (*Test)? -Dtest.name is pretty much how we do 
this currently and I am not sure we want to break this in all CI scripts.

I do not think we are actually winning anything with that approach.

Regards


From: Josh McKenzie mailto:jmcken...@apache.org>>
Sent: Monday, October 31, 2022 12:49
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



To Derek's point, why not go with:

Would it be sufficient to look for files that don't end in "Test.java" but 
contain "@Test" annotations?
+ exclude files that contain "abstract class" in them that matches the file 
name?

Combine that w/checking specific subdirectories and we relax the required 
naming convention quite a bit; makes it easy to do the right thing and hard to 
do the wrong thing.

It's also very simple which is a feature.

On Fri, Oct 28, 2022, at 4:17 AM, Miklosovic, Stefan wrote:
Hi Berenguer,

I agree. We can always go heavy-weight anyway. I will dedicate more time to 
that the next week. It would be nice if you tagged along and help with the 
review.

FYI the ticket is touching all branches we support (there are some skipped 
tests in 3.0 too), we would just add checkstyle to 4.1 and trunk as discussed, 
so I expect we fix all 5 branches at once.

Regards

Stefan


From: Berenguer Blasi 
mailto:berenguerbl...@gmail.com>>>
Sent: Friday, October 28, 2022 9:43
To: 
dev@cassandra.apache.org>
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.




Hi Stefan,

seems none of the 2 proposals are strongly opposed to. If you feel 

Re: Some tests are never executed in CI due to their name

[Josh McKenzie]

So to be clear, I don't really feel super strongly on this, I just thought 
Derek's suggestion seemed clean and elegant. Checkstyle to force a certain 
naming convention on things is fine enough; I certainly wouldn't stand in the 
way. Tooling like that self-educates users (see: Guardrails) so it's pretty low 
friction to the community IMO.

> Secondly, I still do not know how you want to practically implement what you 
> suggest without significantly breaking anything.
It's unclear to me how the combination of "Has @Test, is not an abstract class 
top level matching file, and is inside the test folder" is going to 
significantly break anything. Maybe I'm missing something; would love to learn.

> how to actually be sure that a developer knows that his test not only passed 
> but it was selected to be run
Andres' changes to .circleci/generate.sh mean you're going to see a _repeat run 
of whatever tests you change or add. I think both approaches (heuristic to 
determine if a file contains tests to run vs. checkstyle enforced naming 
convention) are more or less comparable here.

On Mon, Oct 31, 2022, at 12:26 PM, Miklosovic, Stefan wrote:
> I think we are already adhering to them without any effort. Overwhelming 
> majority of devs are naming their tests "just right" and I do not think they 
> are doing that with any significant effort, probably none. We are only 
> re-enforcing the unspoken everybody implicitly agreed on. If somebody does 
> not name their tests like the rest, I think it would raise the red flags 
> already during a review.
> 
> Secondly, I still do not know how you want to practically implement what you 
> suggest without significantly breaking anything.
> 
> I think the crucial question we should ask is how to actually be sure that a 
> developer knows that his test not only passed but it was selected to be run 
> (now based on *Test). How did these tests which skipped as of now end up 
> there in the first place? Somebody just committed them, run the CI and "it 
> was green" so they proceed with the commit. But they did not check it was 
> actually run because "why not?".
> 
> So, we are putting a kind of detector in place to signal that a test is not 
> going to be actually even run when the checkstyle fails.
> 
> 
> From: Josh McKenzie 
> Sent: Monday, October 31, 2022 17:16
> To: dev
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> I do not think that having a test class called "SdsdakASsdadsada" which 
> happen to contain @Test and is not abstract should be considered to be a 
> valid file name / test class
> I mean, I wouldn't recommend it certainly, but I'm not concerned about 
> whether a file is "TestThing" or "ThingTest" or "WeTestThing". The ordering 
> of names on those files adds very little IMO.
> 
> I do not think we are actually winning anything with that approach.
> Well, the name of this thread is "Some tests are never executed in CI due to 
> their name" and it fixes that in a robust way without asking of any behavior 
> changes of the project community to adhere to an arbitrary rigid naming 
> guideline for something. So there's that. :)
> 
> On Mon, Oct 31, 2022, at 8:02 AM, Miklosovic, Stefan wrote:
> I always hit "send" before realizing I have other points to add :)
> 
> I was thinking we want to go with checkstyle and check the name. Not manually 
> parsing the file names. Do not we also want these test files to look 
> consistent? I do not think that having a test class called "SdsdakASsdadsada" 
> which happen to contain @Test and is not abstract should be considered to be 
> a valid file name / test class. Do not we also want to enforce some 
> regularity into this?
> 
> Another point is that even we parsed we so it will be picked up while tests 
> are executed, that means that it will not be checked on the commit?
> 
> Lastly, how do you actually want to do this if we currently have test.name 
> property which is a simple regexp (*Test)? -Dtest.name is pretty much how we 
> do this currently and I am not sure we want to break this in all CI scripts.
> 
> I do not think we are actually winning anything with that approach.
> 
> Regards
> 
> 
> From: Josh McKenzie mailto:jmcken...@apache.org>>
> Sent: Monday, October 31, 2022 12:49
> To: dev
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> To Derek's point, why not go with:
> 
> Would it be sufficient to look for files that don't end in "Test.java" but 
> contain "@Test" annotations?
> + exclude files that contain "abstract class" in them that matches the fi

Re: Some tests are never executed in CI due to their name

[Miklosovic, Stefan]

>> It's unclear to me how the combination of "Has @Test, is not an abstract 
>> class top level matching file, and is inside the test folder" is going to 
>> significantly break anything. Maybe I'm missing something; would love to 
>> learn.

I just don't see where you want to "hook" this step exactly either in the CI 
run or in build.xml or what have you. My wild guess is that this translates to 
some kind of a bash script, right (checking if it has @Test and not abstract) ? 
And then what? So we have test.name property, so your idea is to somehow get 
the list of classes and then you want to feed it to test.name property? That 
property would be very long if we are going to enumerate whole test suite with 
package names as well, not super user friendly if you ask me. I just feel we 
need to plumb this on quite a low level to achieve this. Why not to put a 5 
line checkstyle in place, rename few classes and move on.

I would still love to have file names somehow consistent so they are also 
visually easily recognizable (what is test and what not) in IDE or similar.

Also, I would like to hear opinions of others when it comes to this second idea.


From: Josh McKenzie 
Sent: Monday, October 31, 2022 19:25
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



So to be clear, I don't really feel super strongly on this, I just thought 
Derek's suggestion seemed clean and elegant. Checkstyle to force a certain 
naming convention on things is fine enough; I certainly wouldn't stand in the 
way. Tooling like that self-educates users (see: Guardrails) so it's pretty low 
friction to the community IMO.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.
It's unclear to me how the combination of "Has @Test, is not an abstract class 
top level matching file, and is inside the test folder" is going to 
significantly break anything. Maybe I'm missing something; would love to learn.

how to actually be sure that a developer knows that his test not only passed 
but it was selected to be run
Andres' changes to .circleci/generate.sh mean you're going to see a _repeat run 
of whatever tests you change or add. I think both approaches (heuristic to 
determine if a file contains tests to run vs. checkstyle enforced naming 
convention) are more or less comparable here.

On Mon, Oct 31, 2022, at 12:26 PM, Miklosovic, Stefan wrote:
I think we are already adhering to them without any effort. Overwhelming 
majority of devs are naming their tests "just right" and I do not think they 
are doing that with any significant effort, probably none. We are only 
re-enforcing the unspoken everybody implicitly agreed on. If somebody does not 
name their tests like the rest, I think it would raise the red flags already 
during a review.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.

I think the crucial question we should ask is how to actually be sure that a 
developer knows that his test not only passed but it was selected to be run 
(now based on *Test). How did these tests which skipped as of now end up there 
in the first place? Somebody just committed them, run the CI and "it was green" 
so they proceed with the commit. But they did not check it was actually run 
because "why not?".

So, we are putting a kind of detector in place to signal that a test is not 
going to be actually even run when the checkstyle fails.


From: Josh McKenzie mailto:jmcken...@apache.org>>
Sent: Monday, October 31, 2022 17:16
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



I do not think that having a test class called "SdsdakASsdadsada" which happen 
to contain @Test and is not abstract should be considered to be a valid file 
name / test class
I mean, I wouldn't recommend it certainly, but I'm not concerned about whether 
a file is "TestThing" or "ThingTest" or "WeTestThing". The ordering of names on 
those files adds very little IMO.

I do not think we are actually winning anything with that approach.
Well, the name of this thread is "Some tests are never executed in CI due to 
their name" and it fixes that in a robust way without asking of any behavior 
changes of the project community to adhere to an arbitrary rigid naming 
guideline for something. So there's that. :)

On Mon, Oct 31, 2022, at 8:02 AM, Miklosovic, Stefan wrote:
I always hit "send" before realizing I have other points to add :)

I was thinking we want to go with checkstyle and check the name. Not man

Re: Some tests are never executed in CI due to their name

[Josh McKenzie]

> I just feel we need to plumb this on quite a low level to achieve this. Why 
> not to put a 5 line checkstyle in place, rename few classes and move on.
https://github.com/apache/cassandra-builds/blob/trunk/build-scripts/cassandra-test.sh#L17-L21

# lists all tests for the specific test type
_list_tests() {
local -r classlistprefix="$1"
find "test/$classlistprefix" -name '*Test.java' | sed 
"s;^test/$classlistprefix/;;g" | sort
}

Shouldn't be *that* painful should it?

On Mon, Oct 31, 2022, at 3:21 PM, Miklosovic, Stefan wrote:
> >> It's unclear to me how the combination of "Has @Test, is not an abstract 
> >> class top level matching file, and is inside the test folder" is going to 
> >> significantly break anything. Maybe I'm missing something; would love to 
> >> learn.
> 
> I just don't see where you want to "hook" this step exactly either in the CI 
> run or in build.xml or what have you. My wild guess is that this translates 
> to some kind of a bash script, right (checking if it has @Test and not 
> abstract) ? And then what? So we have test.name property, so your idea is to 
> somehow get the list of classes and then you want to feed it to test.name 
> property? That property would be very long if we are going to enumerate whole 
> test suite with package names as well, not super user friendly if you ask me. 
> I just feel we need to plumb this on quite a low level to achieve this. Why 
> not to put a 5 line checkstyle in place, rename few classes and move on.
> 
> I would still love to have file names somehow consistent so they are also 
> visually easily recognizable (what is test and what not) in IDE or similar.
> 
> Also, I would like to hear opinions of others when it comes to this second 
> idea.
> 
> 
> From: Josh McKenzie 
> Sent: Monday, October 31, 2022 19:25
> To: dev
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> So to be clear, I don't really feel super strongly on this, I just thought 
> Derek's suggestion seemed clean and elegant. Checkstyle to force a certain 
> naming convention on things is fine enough; I certainly wouldn't stand in the 
> way. Tooling like that self-educates users (see: Guardrails) so it's pretty 
> low friction to the community IMO.
> 
> Secondly, I still do not know how you want to practically implement what you 
> suggest without significantly breaking anything.
> It's unclear to me how the combination of "Has @Test, is not an abstract 
> class top level matching file, and is inside the test folder" is going to 
> significantly break anything. Maybe I'm missing something; would love to 
> learn.
> 
> how to actually be sure that a developer knows that his test not only passed 
> but it was selected to be run
> Andres' changes to .circleci/generate.sh mean you're going to see a _repeat 
> run of whatever tests you change or add. I think both approaches (heuristic 
> to determine if a file contains tests to run vs. checkstyle enforced naming 
> convention) are more or less comparable here.
> 
> On Mon, Oct 31, 2022, at 12:26 PM, Miklosovic, Stefan wrote:
> I think we are already adhering to them without any effort. Overwhelming 
> majority of devs are naming their tests "just right" and I do not think they 
> are doing that with any significant effort, probably none. We are only 
> re-enforcing the unspoken everybody implicitly agreed on. If somebody does 
> not name their tests like the rest, I think it would raise the red flags 
> already during a review.
> 
> Secondly, I still do not know how you want to practically implement what you 
> suggest without significantly breaking anything.
> 
> I think the crucial question we should ask is how to actually be sure that a 
> developer knows that his test not only passed but it was selected to be run 
> (now based on *Test). How did these tests which skipped as of now end up 
> there in the first place? Somebody just committed them, run the CI and "it 
> was green" so they proceed with the commit. But they did not check it was 
> actually run because "why not?".
> 
> So, we are putting a kind of detector in place to signal that a test is not 
> going to be actually even run when the checkstyle fails.
> 
> 
> From: Josh McKenzie mailto:jmcken...@apache.org>>
> Sent: Monday, October 31, 2022 17:16
> To: dev
> Subject: Re: Some tests are never executed in CI due to their name
> 
> NetApp Security WARNING: This is an external email. Do not click links or 
> open attachments unless you recognize the sender and know the content is safe.
> 
> 
> 
> I do not think that having a test class called "SdsdakASsdadsada" which 
> happen to contain @Test and is not abstract should be considered to be a 
> valid file name / test class
> I mean, I wouldn'

Re: Some tests are never executed in CI due to their name

[Miklosovic, Stefan]

Finish it all to the completion be if you don't mind. To see it all in action. 
Again, I don't know what to do with this function, sorry. Why is it in builds? 
So standalone build.xml and ant target would be without this or we would need 
to duplicate?

I am by no mean against this exploration, I would like to see it all playing 
together, that is all.


From: Josh McKenzie 
Sent: Monday, October 31, 2022 20:36
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



I just feel we need to plumb this on quite a low level to achieve this. Why not 
to put a 5 line checkstyle in place, rename few classes and move on.
https://github.com/apache/cassandra-builds/blob/trunk/build-scripts/cassandra-test.sh#L17-L21

# lists all tests for the specific test type
_list_tests() {
local -r classlistprefix="$1"
find "test/$classlistprefix" -name '*Test.java' | sed 
"s;^test/$classlistprefix/;;g" | sort
}

Shouldn't be that painful should it?

On Mon, Oct 31, 2022, at 3:21 PM, Miklosovic, Stefan wrote:
>> It's unclear to me how the combination of "Has @Test, is not an abstract 
>> class top level matching file, and is inside the test folder" is going to 
>> significantly break anything. Maybe I'm missing something; would love to 
>> learn.

I just don't see where you want to "hook" this step exactly either in the CI 
run or in build.xml or what have you. My wild guess is that this translates to 
some kind of a bash script, right (checking if it has @Test and not abstract) ? 
And then what? So we have test.name property, so your idea is to somehow get 
the list of classes and then you want to feed it to test.name property? That 
property would be very long if we are going to enumerate whole test suite with 
package names as well, not super user friendly if you ask me. I just feel we 
need to plumb this on quite a low level to achieve this. Why not to put a 5 
line checkstyle in place, rename few classes and move on.

I would still love to have file names somehow consistent so they are also 
visually easily recognizable (what is test and what not) in IDE or similar.

Also, I would like to hear opinions of others when it comes to this second idea.


From: Josh McKenzie mailto:jmcken...@apache.org>>
Sent: Monday, October 31, 2022 19:25
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



So to be clear, I don't really feel super strongly on this, I just thought 
Derek's suggestion seemed clean and elegant. Checkstyle to force a certain 
naming convention on things is fine enough; I certainly wouldn't stand in the 
way. Tooling like that self-educates users (see: Guardrails) so it's pretty low 
friction to the community IMO.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.
It's unclear to me how the combination of "Has @Test, is not an abstract class 
top level matching file, and is inside the test folder" is going to 
significantly break anything. Maybe I'm missing something; would love to learn.

how to actually be sure that a developer knows that his test not only passed 
but it was selected to be run
Andres' changes to .circleci/generate.sh mean you're going to see a _repeat run 
of whatever tests you change or add. I think both approaches (heuristic to 
determine if a file contains tests to run vs. checkstyle enforced naming 
convention) are more or less comparable here.

On Mon, Oct 31, 2022, at 12:26 PM, Miklosovic, Stefan wrote:
I think we are already adhering to them without any effort. Overwhelming 
majority of devs are naming their tests "just right" and I do not think they 
are doing that with any significant effort, probably none. We are only 
re-enforcing the unspoken everybody implicitly agreed on. If somebody does not 
name their tests like the rest, I think it would raise the red flags already 
during a review.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.

I think the crucial question we should ask is how to actually be sure that a 
developer knows that his test not only passed but it was selected to be run 
(now based on *Test). How did these tests which skipped as of now end up there 
in the first place? Somebody just committed them, run the CI and "it was green" 
so they proceed with the commit. But they did not check it was actually run 
because "why not?".

So, we are putting a kind of detector in place to signal that a test is not 
going to be actually even run when the checkstyle fails.

_

Re: Some tests are never executed in CI due to their name

[Miklosovic, Stefan]

Aha wait a minute I got it :D Yeah. This is not what you invented, it is 
already in, sorry.

So, you are saying we should just modify this to cover @Test / abstract.

I dont know ... interesting.

Lets wait for others to answer.


From: Josh McKenzie 
Sent: Monday, October 31, 2022 20:36
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



I just feel we need to plumb this on quite a low level to achieve this. Why not 
to put a 5 line checkstyle in place, rename few classes and move on.
https://github.com/apache/cassandra-builds/blob/trunk/build-scripts/cassandra-test.sh#L17-L21

# lists all tests for the specific test type
_list_tests() {
local -r classlistprefix="$1"
find "test/$classlistprefix" -name '*Test.java' | sed 
"s;^test/$classlistprefix/;;g" | sort
}

Shouldn't be that painful should it?

On Mon, Oct 31, 2022, at 3:21 PM, Miklosovic, Stefan wrote:
>> It's unclear to me how the combination of "Has @Test, is not an abstract 
>> class top level matching file, and is inside the test folder" is going to 
>> significantly break anything. Maybe I'm missing something; would love to 
>> learn.

I just don't see where you want to "hook" this step exactly either in the CI 
run or in build.xml or what have you. My wild guess is that this translates to 
some kind of a bash script, right (checking if it has @Test and not abstract) ? 
And then what? So we have test.name property, so your idea is to somehow get 
the list of classes and then you want to feed it to test.name property? That 
property would be very long if we are going to enumerate whole test suite with 
package names as well, not super user friendly if you ask me. I just feel we 
need to plumb this on quite a low level to achieve this. Why not to put a 5 
line checkstyle in place, rename few classes and move on.

I would still love to have file names somehow consistent so they are also 
visually easily recognizable (what is test and what not) in IDE or similar.

Also, I would like to hear opinions of others when it comes to this second idea.


From: Josh McKenzie mailto:jmcken...@apache.org>>
Sent: Monday, October 31, 2022 19:25
To: dev
Subject: Re: Some tests are never executed in CI due to their name

NetApp Security WARNING: This is an external email. Do not click links or open 
attachments unless you recognize the sender and know the content is safe.



So to be clear, I don't really feel super strongly on this, I just thought 
Derek's suggestion seemed clean and elegant. Checkstyle to force a certain 
naming convention on things is fine enough; I certainly wouldn't stand in the 
way. Tooling like that self-educates users (see: Guardrails) so it's pretty low 
friction to the community IMO.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.
It's unclear to me how the combination of "Has @Test, is not an abstract class 
top level matching file, and is inside the test folder" is going to 
significantly break anything. Maybe I'm missing something; would love to learn.

how to actually be sure that a developer knows that his test not only passed 
but it was selected to be run
Andres' changes to .circleci/generate.sh mean you're going to see a _repeat run 
of whatever tests you change or add. I think both approaches (heuristic to 
determine if a file contains tests to run vs. checkstyle enforced naming 
convention) are more or less comparable here.

On Mon, Oct 31, 2022, at 12:26 PM, Miklosovic, Stefan wrote:
I think we are already adhering to them without any effort. Overwhelming 
majority of devs are naming their tests "just right" and I do not think they 
are doing that with any significant effort, probably none. We are only 
re-enforcing the unspoken everybody implicitly agreed on. If somebody does not 
name their tests like the rest, I think it would raise the red flags already 
during a review.

Secondly, I still do not know how you want to practically implement what you 
suggest without significantly breaking anything.

I think the crucial question we should ask is how to actually be sure that a 
developer knows that his test not only passed but it was selected to be run 
(now based on *Test). How did these tests which skipped as of now end up there 
in the first place? Somebody just committed them, run the CI and "it was green" 
so they proceed with the commit. But they did not check it was actually run 
because "why not?".

So, we are putting a kind of detector in place to signal that a test is not 
going to be actually even run when the checkstyle fails.


From: Josh McKenzie 
mailto:jmcken...@apache.org>