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

[test][NFC] Change from nose to pytest (web library) #3932

Merged
merged 5 commits into from
Jun 20, 2023

Conversation

Szelethus
Copy link
Collaborator

@Szelethus Szelethus commented Jun 14, 2023

Continuation of #3931 and #3926.

For your convenience, I split this patch up into several parts -- in light of the previous patches, the first two commits shouldn't offer anything new.

edit: As a note, pytest doesn't support package level setup/teardown, only class level. Packages where only a single class is present, these are equivalent. This is why the 2nd commit isn't particularly exciting. :)

The 3rd one needs a little explanation:

  • web/tes/funtional/authentication/:
    • This test creates a temporary server. Previously, this was done for the entire directory -- create a server, run all tests within the directory, and then close it. Since per-package level setup/teardown isn't a thing in pytest, I changed this to per-class. However, it turns out that we didn't tear down the temporary server properly. The server is created by spawning a multiprocessing thread that listens to an event. If the event is set, the thread (and the CodeChecker server inside it) will be terminated, however, that event needs to be cleared -- otherwise it will still be set by the time the server would be reopened, and as a result, would immediately close.
    • The tests depended one another, and messed with each other, as indicated by my comments. These being separated is just a 100% win.
  • The others don't stand out. They all connect to a CodeChecker server shared across all tests. Previously, per-package, in the setup, they created a product, and in the teardown, they removed that product. Now that this has to be done per-class, we stumbled across a bug -- if a product is removed, the endline wouldn't be freed up for it. As a result, we can't just create and remove the same product over and over again, and need to create a new product for each class in the package.

The 4th one is something I still need some fixing on. The only reason I haven't quite done it yet, because I didn't break it, it was broken already, so it feels appropriate to address it in a separate patch :)

@Szelethus Szelethus added test ☑️ Adding or refactoring tests refactoring 😡 ➡️ 🙂 Refactoring code. dependencies 📦 Pull requests that update a dependency file labels Jun 14, 2023
@Szelethus Szelethus added this to the release 6.22.2 milestone Jun 14, 2023
@Szelethus Szelethus force-pushed the nose_to_pytest_web_granular branch from cc2bdec to d27c67f Compare June 14, 2023 12:23
@Szelethus Szelethus changed the title Nose to pytest web granular [test][NFC] Change from nose to pytest (web library) Jun 14, 2023
sys.path.append(REPO_ROOT)
sys.path.append(os.path.join(REPO_ROOT, 'web'))
sys.path.append(os.path.join(
REPO_ROOT, 'analyzer', 'tools', 'statistics_collector'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this tool here in a web-related makefile? The analyzer and web related parts of CodeChecker are supposed to be hermetically separated.

# --maxfail=1

# do not capture stdout
--capture=fd
Copy link
Contributor

Choose a reason for hiding this comment

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

In analyzer tests this option was --capture=sys. Shouldn't they match?

Copy link
Collaborator

@vodorok vodorok left a comment

Choose a reason for hiding this comment

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

LGTM

@vodorok
Copy link
Collaborator

vodorok commented Jun 20, 2023

One note, It seems that the pr contains another commit, [ldlogger] Recognize linux_spawn alongside exec* calls, please rebase before merging.

@Szelethus Szelethus force-pushed the nose_to_pytest_web_granular branch from d27c67f to 3aa94a8 Compare June 20, 2023 09:43
@Szelethus Szelethus requested a review from bruntib June 20, 2023 10:23
@bruntib bruntib merged commit e3c4c96 into Ericsson:master Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Pull requests that update a dependency file refactoring 😡 ➡️ 🙂 Refactoring code. test ☑️ Adding or refactoring tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants