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

Refactor tests and benchmarks #4700

Merged
merged 10 commits into from
Jun 24, 2023

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Jun 22, 2023

Description

This PR makes a few changes:

  • Tests and benchmarks run on the main thread

    • This caused network tests to fail, since they rely on the Qt event-loop. These were adjusted to use qWaitFor which keeps the event-loop spinning. Since this is actually declared in QtCore, no dependency to the Test component is necessary.
  • Running benchmarks/tests related to emojis would segfault (at least for me with MSVC), because the ImageExpirationPool would be destroyed before the Image from getEmpty (the destructor calls removeImagePtr).

    The fix for this is a bit more involved, as it's not trivial to control the order of destruction of static inside functions, so the statics are moved inside an anonymous namespace.
    Creating a ImageExpirationPool when starting up will cause the freeTimer_ to not work, as the main thread isn't running yet. That's why it's created lazily.

    For the emoji tests/benchmarks, this will cause QObject::startTimer: Timers can only be used with threads started with QThread to be printed after they're finished, because that's the first time the ImageExpirationPool is accessed (when destroying the empty image). This can be fixed by e.g. calling ImageExpirationPool::instance() somewhere before/during the tests/benchmarks, but it's not really an issue here, since we're about to destroy the pool regardless (thus the timer would've never fired).

  • The tests for filters failed on Qt 6 (filters are probably not fully working in Qt 6, so this could be a separate PR).

  • https://httpbin.org was really slow for me at times, and sometimes it even timed out. I found https://postman-echo.com, which has the same API except for the special 402 status response.

Now the following is possible:

  • Running all tests in debug mode (this means that assertions will fire!).
  • Using ctest.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Copy link
Contributor Author

@Nerixyz Nerixyz left a comment

Choose a reason for hiding this comment

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

and use ctest

It doesn't actually use ctest - but it could: ctest -j$(nproc) --output-on-failure (optionally with -T test).

tests/src/NetworkRequest.cpp Outdated Show resolved Hide resolved
tests/src/NetworkRequest.cpp Show resolved Hide resolved
tests/src/NetworkRequest.cpp Outdated Show resolved Hide resolved
src/messages/Image.cpp Outdated Show resolved Hide resolved
This test was really flaky for me -
it probably depends a lot on the scheduling
and the system this is running on.
This reverts commit 86dc1f4.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/Image.cpp Show resolved Hide resolved
tests/src/NetworkRequest.cpp Show resolved Hide resolved
tests/src/NetworkRequest.cpp Show resolved Hide resolved
@pajlada pajlada merged commit b9934a4 into Chatterino:master Jun 24, 2023
14 checks passed
@Nerixyz Nerixyz deleted the refactor/test-benchmark branch June 24, 2023 13:09
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