spark-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hyukjin Kwon <gurwls...@gmail.com>
Subject Re: Adding JIRA ID as the prefix for the test case name
Date Fri, 15 Nov 2019 02:03:02 GMT
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>님이
작성:

> 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> wrote:
>
>> I opened a PR - https://github.com/apache/spark-website/pull/231
>>
>> 2019년 11월 13일 (수) 오전 10:43, Hyukjin Kwon <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>님이
작성:
>>>
>>>> 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>
>>>> 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