spark-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Felix Cheung <felixcheun...@hotmail.com>
Subject Re: Adding JIRA ID as the prefix for the test case name
Date Fri, 15 Nov 2019 06:51:36 GMT
this is about test description and not test file name right?

if yes I don’t see a problem.

________________________________
From: Hyukjin Kwon <gurwls223@gmail.com>
Sent: Thursday, November 14, 2019 6:03:02 PM
To: Shixiong(Ryan) Zhu <shixiong@databricks.com>
Cc: dev <dev@spark.apache.org>; Felix Cheung <felixcheung_m@hotmail.com>; Shivaram
Venkataraman <shivaram@eecs.berkeley.edu>
Subject: Re: Adding JIRA ID as the prefix for the test case name

Yeah, sounds good to have it.

In case of R, it seems not quite common to write down JIRA ID [1] but looks some have the
prefix in its test name in general.
In case of Python and Java, seems we time to time write a JIRA ID in the comment right under
the test method [2][3].

Given this pattern, I would like to suggest use the same format but:

1. For Python and Java, write a single comment that starts with JIRA ID and short description,
e.g. (SPARK-XXXXX: test blah blah)
2. For R, use JIRA ID as a prefix for its test name.

[1] git grep -r "SPARK-" -- '*test*.R'
[2] git grep -r "SPARK-" -- '*Suite.java'
[3] git grep -r "SPARK-" -- '*test*.py'

Does that make sense? Adding Felix and Shivaram too.


2019년 11월 15일 (금) 오전 3:13, Shixiong(Ryan) Zhu <shixiong@databricks.com<mailto:shixiong@databricks.com>>님이
작성:
Should we also add a guideline for non Scala tests? Other languages (Java, Python, R) don't
support using string as a test name.

Best Regards,

Ryan


On Thu, Nov 14, 2019 at 4:04 AM Hyukjin Kwon <gurwls223@gmail.com<mailto:gurwls223@gmail.com>>
wrote:
I opened a PR - https://github.com/apache/spark-website/pull/231

2019년 11월 13일 (수) 오전 10:43, Hyukjin Kwon <gurwls223@gmail.com<mailto:gurwls223@gmail.com>>님이
작성:
> In general a test should be self descriptive and I don't think we should be adding JIRA
ticket references wholesale. Any action that the reader has to take to understand why a test
was introduced is one too many. However in some cases the thing we are trying to test is very
subtle and in that case a reference to a JIRA ticket might be useful, I do still feel that
this should be a backstop and that properly documenting your tests is a much better way of
dealing with this.

Yeah, the test should be self-descriptive. I don't think adding a JIRA prefix harms this point.
Probably I should add this sentence in the guidelines as well.
Adding a JIRA prefix just adds one extra hint to track down details. I think it's fine to
stick to this practice and make it simpler and clear to follow.

> 1. what if multiple JIRA IDs relating to the same test? we just take the very first JIRA
ID?
Ideally one JIRA should describe one issue and one PR should fix one JIRA with a dedicated
test.
Yeah, I think I would take the very first JIRA ID.

> 2. are we going to have a full scan of all existing tests and attach a JIRA ID to it?
Yea, let's don't do this.

> It's a nice-to-have, not super essential, just because ...
It's been asked multiple times and each committer seems having a different understanding on
this.
It's not a biggie but wanted to make it clear and conclude this.

> I'd add this only when a test specifically targets a certain issue.
Yes, so this one I am not sure. From what I heard, people adds the JIRA in cases below:

- Whenever the JIRA type is a bug
- When a PR adds a couple of tests
- Only when a test specifically targets a certain issue.
- ...

Which one do we prefer and simpler to follow?

Or I can combine as below (im gonna reword when I actually document this):
1. In general, we should add a JIRA ID as prefix of a test when a PR targets to fix a specific
issue.
    In practice, it usually happens when a JIRA type is a bug or a PR adds a couple of tests.
2. Uses "SPARK-XXXX: test name" format

If we have no objection with ^, let me go with this.

2019년 11월 13일 (수) 오전 8:14, Sean Owen <srowen@gmail.com<mailto:srowen@gmail.com>>님이
작성:
Let's suggest "SPARK-12345:" but not go back and change a bunch of test cases.
I'd add this only when a test specifically targets a certain issue.
It's a nice-to-have, not super essential, just because in the rare
case you need to understand why a test asserts something, you can go
back and find what added it in the git history without much trouble.

On Mon, Nov 11, 2019 at 10:46 AM Hyukjin Kwon <gurwls223@gmail.com<mailto:gurwls223@gmail.com>>
wrote:
>
> Hi all,
>
> Maybe it's not a big deal but it brought some confusions time to time into Spark dev
and community. I think it's time to discuss about when/which format to add a JIRA ID as a
prefix for the test case name in Scala test cases.
>
> Currently we have many test case names with prefixes as below:
>
> test("SPARK-XXXXX blah blah")
> test("SPARK-XXXXX: blah blah")
> test("SPARK-XXXXX - blah blah")
> test("[SPARK-XXXXX] blah blah")
> …
>
> It is a good practice to have the JIRA ID in general because, for instance,
> it makes us put less efforts to track commit histories (or even when the files
> are totally moved), or to track related information of tests failed.
> Considering Spark's getting big, I think it's good to document.
>
> I would like to suggest this and document it in our guideline:
>
> 1. Add a prefix into a test name when a PR adds a couple of tests.
> 2. Uses "SPARK-XXXX: test name" format which is used in our code base most
>       often[1].
>
> We should make it simple and clear but closer to the actual practice. So, I would like
to listen to what other people think. I would appreciate if you guys give some feedback about
when to add the JIRA prefix. One alternative is that, we only add the prefix when the JIRA's
type is bug.
>
> [1]
> git grep -E 'test\("\SPARK-([0-9]+):' | wc -l
>      923
> git grep -E 'test\("\SPARK-([0-9]+) ' | wc -l
>      477
> git grep -E 'test\("\[SPARK-([0-9]+)\]' | wc -l
>       16
> git grep -E 'test\("\SPARK-([0-9]+) -' | wc -l
>       13
>
>
>
Mime
View raw message