Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up no longer present test suites from _build #2626

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

4tyTwo
Copy link

@4tyTwo 4tyTwo commented Oct 9, 2021

This PR is an attempt to fix a bug described in #2377

The bug causes rebar3 to execute or attempt to execute ct suites, that has been renamed or removed in the source code as there is no orphan file cleanup

The chosen approach has limitations. Perhaps utilizing the code in compile-related modules would be a more robust solution, but I feel like I would need a lot more time and some guidance to make a change there.

One of the discovered limitations of this approach is inability to handle duplicate module names. If two suites in different directories have the same name, deleting/renaming one of them would still leave it's object file in respective _build directory.

The PR has no tests yet, as I found it difficult to reproduce the bug conditions within tests. Help would be appreciated

I would appreciate knowing if for now we can advance with the proposed approach or the bug in question requires a more robust solution

@4tyTwo 4tyTwo force-pushed the fix/dont-run-deleted-or-renamed-tests-suites branch from 185337e to 21a2ce0 Compare October 10, 2021 11:58
@4tyTwo
Copy link
Author

4tyTwo commented Oct 18, 2021

@ferd can I get another build trigger? I've fixed the dialyzer issue found by the first one

{ok, T} = Tests,
TestSources = proplists:get_value(dir, T, []),
AllDeps = rebar_state:code_paths(S, all_deps),
IsTestDir = fun(Path) -> string:slice(Path, length(Path) - 4, 4) == "test" end,
Copy link
Collaborator

@ferd ferd Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test you're looking here though might be the profile of the path, which would not be accurate in all cases. For example, you might get test+demo if you ran rebar3 as demo ct

We don't run tests on deps, just on top-level apps, which can be found with rebar_state:project_apps(State) (which you can then expand with rebar_app_info:out_dir/1 to get the app's path in _build), so that should cut down the search space.

Additionally, in umbrella applications where a top-level test/ directory exists, the path should be _build/<profile>/extras/test/.

The last final weird case is visible in rebar3. We have some integration suites that run only on Linux that can be called by running rebar3 as systest ct. The directories can be seen in:

→ ls _build/systest+test/lib/rebar/
ebin  include  priv  src  systest  test

where test cases exist both in systest/ and in test/.

Common test integration is really messy, and my understanding/idea is that we should be able to take the {dir, ...} entries and plug them onto the out_dir(AppInfo) result to get all the entries, plus the extras path if it exists.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding/idea is that we should be able to take the {dir, ...} entries and plug them onto the out_dir(AppInfo)

While I can see it working, I can't think of an actually nice way of doing it. I'll try giving it another thought again

This last case when it is possible to provide arbitrary directory which will contain test suites is kind of tricky to work around. My immediate solution would be just looking for .*_SUITE.erl files recursively in both extras and _build/PROFILE/lib/APP (excluding ebin, src, priv and include I guess).
Can you spot any obvious issues with that? My concern is that I'm not sure if any not test .erl files can exist outside the aforementioned 4 directories and therefore be in danger of being affected

Copy link
Collaborator

@ferd ferd Nov 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, anything that specifies paths in src_dirs or extra_src_dirs can contain valid Erlang files. It is configurable for each project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants