Delete duplicates of unit tests #5461

vkatsuba · 2021-11-29T23:30:56Z

Remove duplicates in tests before run unit.
This is need for case if in tests will be used duplicates
with/without prefix, example:

Input - [issue_tests, issue]
Output - [issue]

Related to #3064

bjorng

Thanks for your pull request.

This is not a complete review, just a few comments about some things that I think could be improved in the code.

I think that you'll need to add a test case to ensure that your duplication removal works correctly and keeps working.

lib/eunit/src/eunit.erl

IngelaAndin · 2021-12-17T15:03:07Z

This does not seem to break anything but as @bjorng pointed out we would like to have a test to ensure that it works and keeps working.

vkatsuba · 2021-12-17T15:12:08Z

One test case is added. Could you please help me understand - what other tests you expected from my side?

IngelaAndin · 2021-12-17T16:25:31Z

Oh, now I see the test. I expect the test to be in the test suite and not in the src code.

eunit/test/eunit_SUITE.erl

vkatsuba · 2021-12-17T16:29:40Z

Should I move this test to eunit_SUITE.erl or we can keep it as is?

IngelaAndin · 2021-12-17T16:33:38Z

Please move it.

vkatsuba · 2021-12-22T12:08:26Z

@IngelaAndin done.

u3s · 2021-12-29T09:34:26Z

i browsed the code related to this functionality and seems to me that it is not tested that well.
could you extend the test case a bit further, so we can get a better coverage and control over change introduced?

I'm thinking about following inputs to eunit:test calls (apart from input already proposed):

eunit
eunit_tests
[eunit]
[eunit_tests]
[eunit,eunit]
[eunit,eunit_tests]
[eunit_tests, eunit]
[eunit_tests, eunit_tests]

I assume that should be quite simple, if some generic code is brought into test case. Maybe list above could be a list of tuples combining {TestInput, ExpectedExecutionResults} and then consumed by test case ...

Having a dummy Eunit test in eunit module (location as originally proposed but for example returning ok) might be a good idea. Not for the purpose of executing it directly, but for the purpose of using it as an input to test case now located in eunit_SUITE and verifying the behavior.

Remove duplicates in tests before run unit. This is need for case if in tests will be used duplicates with/without prefix, example: * Input - [issue, issue_tests] * Output - [issue]

vkatsuba · 2021-12-29T12:05:33Z

@u3s many thanks! Yep, I'm miss of some test points. Fixed.

u3s · 2021-12-30T09:38:08Z

please check shell output below - code from your PR and the base commit was used
eunit_tests contains 7 tests and eunit contains 1 test (which I added manually
for both commits)
should test from eunit module be executed if user asks for tests from eunit_tests (I think it worked differently for you previous commit 34f6de4)?
could eunit_duplicates_test be improved so it controls this behavior?
regarding changes in eunit module - I plan to discuss with the team if we're
fine with the proposal or would like some different approach. I will let you know
next week.
However, improving the test case is something that would be good no matter
what discussion output will.

**** test added to eunit module
git diff
diff --git a/lib/eunit/src/eunit.erl b/lib/eunit/src/eunit.erl
index 5b9ead74e9..db6dc20103 100644
--- a/lib/eunit/src/eunit.erl
+++ b/lib/eunit/src/eunit.erl
@@ -302,3 +302,6 @@ prepare_candidate(Test) ->
         false ->
             list_to_atom(Name)
     end.
+
+dummy_test() ->
+    ok.

**** 32d401265d (with test added to eunit)
1> eunit:test(eunit).
  All 8 tests passed.
ok
2> eunit:test(eunit_tests).
  All 7 tests passed.
ok
3> eunit:test([eunit]).
  All 8 tests passed.
ok
4> eunit:test([eunit_tests]).
  All 7 tests passed.
ok
5> eunit:test([eunit,eunit]).
  All 16 tests passed.
ok
6> eunit:test([eunit,eunit_tests]).
  All 15 tests passed.
ok
7> eunit:test([eunit_tests, eunit]).
  All 15 tests passed.
ok
8> eunit:test([eunit_tests, eunit_tests]).
  All 14 tests passed.
ok

**** 4bf53f3e10 (with test added to eunit)
1> eunit:test(eunit).
  All 8 tests passed.
ok
2> eunit:test(eunit_tests).
  All 7 tests passed.
ok
3> eunit:test([eunit]).
  All 8 tests passed.
ok
4> eunit:test([eunit_tests]).
  All 8 tests passed.
ok
5> eunit:test([eunit,eunit]).
  All 8 tests passed.
ok
6> eunit:test([eunit,eunit_tests]).
  All 8 tests passed.
ok
7> eunit:test([eunit_tests, eunit]).
  All 8 tests passed.
ok
8> eunit:test([eunit_tests, eunit_tests]).
  All 8 tests passed.
ok

vkatsuba · 2021-12-30T10:30:21Z

The output looks exactly as I expected(as it was difficult for me to choose what to leave on the list - so I left without a prefix - the main issue was - what name should be prioritized with or without prefix during duplication?).

Yes, previous implementations was work as
input [a, a_tests]
output [a]

input [a_tests, a]
output [a_tests]

But as I mention before it was difficult for me to determine the priority of tests because of this, I gave priority to modules without a prefix.

But maybe this is not what your team expects, so, I will waiting for feedback in next week and maybe it might be worth implementing a different approach. Thanks.

P.S. To be honest, I don't really like this approach either which is provided in current PR, maybe need change it as:
1 - Detect duplicates and remove all duplicates with and without prefixes
2 - Check if module with and without prefixes is have test cases
3 - if one of the modules contains tests and the other does not, give priority to module with tests
4 - If two modules have tests - collect them in one place and run it

u3s · 2022-01-04T13:43:20Z

Summary of discussion we had about this change.
Currently PR proposes 2 adjustments to become new default execution flow in Eunit framework.
For the discussion purpose, I will describe each of them separately - please comment if I'm missing something.

OPTION 1

is about reverting side effects of EUnit heuristic resulting with execution of tests found in modules fred, fred_tests if eunit:test(fred) or eunit:test([fred]) is called
is probably the root problem reported in ERL-97: eunit run twice tests when both module and module_tests given #3064
I'm presuming some build systems or libraries might prefer call eunit:test on all modules they manage to find without analyzing their content or relation between filenames. And not expecting execution containing tests from fred, fred_tests, fred_tests if they call eunit:test([fred, fred_tests])
instead execution containing tests from fred, fred_tests if they call eunit:test([fred, fred_tests]) could be preferred

OPTION 2

is about removing duplicate items on the input list - so if eunit:test([eunit, eunit, eunit]) is called execution flow happens as if user called eunit:test([eunit])

Proposal for OPTION 1

leave legacy execution flow unchanged
add new option for eunit:test/2 - so that users who don't like the heuristic can disable it
when used, new option should result with not executing related code in eunit_data:get_module_tests1/2
it would be nice to have it documented
it would be nice to have a test(or tests) covering heuristic enabled (default) and disabled(when new option used)

Proposal for OPTION 2

we are having doubts about it - maybe it was not described precisely enough
firstly, some of the users might see a purpose in having duplicates in the input list
secondly, we are not sure if it is a problem reported in ERL-97: eunit run twice tests when both module and module_tests given #3064, although maybe some of the comments could suggest that direction
thirdly, we are not sure if implementing it in eunit is the best choice - simple implementation in eunit module might be a list processing that users can do themselves if wish to avoid that mistakes, more complex implementation in eunit_data might be not worth the work on the other hand ... depends on how the feature would be specified ...
we would like to skip it for now - unless you have a clear view on what the functionality should be and would like to discuss that

vkatsuba · 2022-01-05T12:16:43Z

@u3s many thanks for feedback! I need spend some time to take a look into it a little big closer. As soon as I have any updates, I will share them here or update the PR with separate commits.

u3s · 2022-01-19T13:35:14Z

any updates?

vkatsuba · 2022-01-19T13:38:27Z

any updates?

Was need to switch to other stuff. I hope will try to improve this PR till end of next week.

u3s · 2022-02-15T16:22:53Z

please let us know if you plan to work on that one or you have different priorities right now and would like us to take over this one.

vkatsuba · 2022-02-15T16:56:24Z

please let us know if you plan to work on that one or you have different priorities right now and would like us to take over this one.

Yes, I want to work on it. But at the same time I have some important tasks which has become a little more lately than I expected ... So, if you feel that it would be more productive if your team took on this task - please go ahead. If by some reason we will provide implementations at the same time, I will not insist on the adoption of my PR - because it doesn't matter who fixes it, what matters is that the fix will provided 🙃.

u3s · 2022-02-15T17:04:06Z

OK. I will let you know if I start working on it, to reduce risk of unnecessary duplicated work.

u3s · 2022-09-02T06:53:37Z

This PR was replaced with #6157 which implements description from section "Proposal for OPTION 1" above.
Thanks for input and discussions which lead to improving eunit behavior. Sorry for delays.

vkatsuba · 2022-09-02T07:27:43Z

Great news! Many thanks for let me know!

bjorng added the team:PS Assigned to OTP team PS label Nov 30, 2021

bjorng linked an issue Nov 30, 2021 that may be closed by this pull request

ERL-97: eunit run twice tests when both module and module_tests given #3064

Closed

bjorng reviewed Nov 30, 2021

View reviewed changes

lib/eunit/src/eunit.erl Outdated Show resolved Hide resolved

lib/eunit/src/eunit.erl Outdated Show resolved Hide resolved

lib/eunit/src/eunit.erl Outdated Show resolved Hide resolved

lib/eunit/src/eunit.erl Outdated Show resolved Hide resolved

vkatsuba force-pushed the fix/gha-3064 branch from 71c50af to 5b8c69a Compare December 2, 2021 23:05

vkatsuba requested a review from bjorng December 2, 2021 23:07

vkatsuba force-pushed the fix/gha-3064 branch 2 times, most recently from cb7069e to 34f6de4 Compare December 3, 2021 10:32

IngelaAndin added the testing currently being tested, tag is used by OTP internal CI label Dec 7, 2021

IngelaAndin assigned IngelaAndin and u3s Dec 14, 2021

vkatsuba force-pushed the fix/gha-3064 branch 3 times, most recently from 7daa637 to c3d4a4a Compare December 22, 2021 10:52

Delete duplicates of unit tests

4bf53f3

Remove duplicates in tests before run unit. This is need for case if in tests will be used duplicates with/without prefix, example: * Input - [issue, issue_tests] * Output - [issue]

vkatsuba force-pushed the fix/gha-3064 branch from c3d4a4a to 4bf53f3 Compare December 29, 2021 12:03

IngelaAndin added waiting waiting for changes/input from author and removed testing currently being tested, tag is used by OTP internal CI labels Jan 11, 2022

IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Mar 9, 2022

u3s mentioned this pull request Jul 15, 2022

eunit: exact execution option #6157

Merged

u3s closed this Sep 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete duplicates of unit tests #5461

Delete duplicates of unit tests #5461

vkatsuba commented Nov 29, 2021 •

edited

Loading

bjorng left a comment

IngelaAndin commented Dec 17, 2021

vkatsuba commented Dec 17, 2021

IngelaAndin commented Dec 17, 2021

vkatsuba commented Dec 17, 2021

IngelaAndin commented Dec 17, 2021

vkatsuba commented Dec 22, 2021

u3s commented Dec 29, 2021 •

edited

Loading

vkatsuba commented Dec 29, 2021

u3s commented Dec 30, 2021 •

edited

Loading

vkatsuba commented Dec 30, 2021 •

edited

Loading

u3s commented Jan 4, 2022 •

edited

Loading

vkatsuba commented Jan 5, 2022

u3s commented Jan 19, 2022

vkatsuba commented Jan 19, 2022

u3s commented Feb 15, 2022

vkatsuba commented Feb 15, 2022

u3s commented Feb 15, 2022

u3s commented Sep 2, 2022

vkatsuba commented Sep 2, 2022

Delete duplicates of unit tests #5461

Delete duplicates of unit tests #5461

Conversation

vkatsuba commented Nov 29, 2021 • edited Loading

bjorng left a comment

Choose a reason for hiding this comment

IngelaAndin commented Dec 17, 2021

vkatsuba commented Dec 17, 2021

IngelaAndin commented Dec 17, 2021

vkatsuba commented Dec 17, 2021

IngelaAndin commented Dec 17, 2021

vkatsuba commented Dec 22, 2021

u3s commented Dec 29, 2021 • edited Loading

vkatsuba commented Dec 29, 2021

u3s commented Dec 30, 2021 • edited Loading

vkatsuba commented Dec 30, 2021 • edited Loading

u3s commented Jan 4, 2022 • edited Loading

vkatsuba commented Jan 5, 2022

u3s commented Jan 19, 2022

vkatsuba commented Jan 19, 2022

u3s commented Feb 15, 2022

vkatsuba commented Feb 15, 2022

u3s commented Feb 15, 2022

u3s commented Sep 2, 2022

vkatsuba commented Sep 2, 2022

vkatsuba commented Nov 29, 2021 •

edited

Loading

u3s commented Dec 29, 2021 •

edited

Loading

u3s commented Dec 30, 2021 •

edited

Loading

vkatsuba commented Dec 30, 2021 •

edited

Loading

u3s commented Jan 4, 2022 •

edited

Loading