Let me document as below in few days: 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.
assuming everybody is happy. 2019년 11월 18일 (월) 오전 11:36, Hyukjin Kwon <gurwls...@gmail.com>님이 작성: > Actually there are not so many Java test cases in Spark (because Scala > runs on JVM as everybody knows)[1]. > > Given that, I think we can avoid to put some efforts on this for now .. I > don't mind if somebody wants to give a shot since it looks good anyway but > to me I wouldn't spend so much time on this .. > > Let me just go ahead as I suggested if you don't mind. Anyone can give a > shot for Display Name - I'm willing to actively review and help. > > [1] > git ls-files '*Suite.java' | wc -l > 172 > git ls-files '*Suite.scala' | wc -l > 1161 > > 2019년 11월 18일 (월) 오전 3:27, Steve Loughran <ste...@cloudera.com>님이 작성: > >> Test reporters do often contain some assumptions about the characters in >> the test methods. Historically JUnit XML reporters have never sanitised the >> method names so XML injection attacks have been fairly trivial. Haven't >> tried this for a while. >> >> That whole JUnit XML report "standard" was actually put together in the >> Ant project with <Junitreport> doing the postprocessing of the JUnit run. >> It was driven by the team's XSL skills than any overreaching strategic goal >> about how to present test results of tests which could run for hours and >> whose output you would really want to aggregate the locks from multiple >> machines and processes and present in awake you can actually navigate. With >> hindsight, a key failing is that we chose to store the test summaries (test >> count, failure count...) as attributes on the root XML mode. Which is why >> the whole DOM gets built up in the JUnit runner. Which is why when that >> JUnit process crashes, you get no report at all. >> >> It'd be straightforward to fix -except too much relies on that file >> now...important things will break. And the maven runner has historically >> never supported custom reporters, to let you experiment with it. >> >> Maybe this is an opportunity to change things. >> >> On Sun, Nov 17, 2019 at 1:42 AM Hyukjin Kwon <gurwls...@gmail.com> wrote: >> >>> DisplayName looks good in general but actually here I would like first >>> to find a existing pattern to document in guidelines given the actual >>> existing practice we all are used to. I'm trying to be very conservative >>> since this guidelines affect everybody. >>> >>> I think it might be better to discuss separately if we want to change >>> what we have been used to. >>> >>> Also, using arbitrary names might not be actually free due to such bug >>> like https://github.com/apache/spark/pull/25630 . It will need some >>> more efforts to investigate as well. >>> >>> On Fri, 15 Nov 2019, 20:56 Steve Loughran, <ste...@cloudera.com.invalid> >>> wrote: >>> >>>> Junit5: Display names. >>>> >>>> Goes all the way to the XML. >>>> >>>> >>>> https://junit.org/junit5/docs/current/user-guide/#writing-tests-display-names >>>> >>>> On Thu, Nov 14, 2019 at 6:13 PM Shixiong(Ryan) Zhu < >>>> shixi...@databricks.com> wrote: >>>> >>>>> 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 <gurwls...@gmail.com> >>>>> wrote: >>>>> >>>>>> I opened a PR - https://github.com/apache/spark-website/pull/231 >>>>>> >>>>>> 2019년 11월 13일 (수) 오전 10:43, Hyukjin Kwon <gurwls...@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 <sro...@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 <gurwls...@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 >>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> >>>>>>>
