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

Ensure order of search history entries in tests #7670

Merged
merged 4 commits into from
Jan 21, 2022

Conversation

XiangRongLin
Copy link
Collaborator

@XiangRongLin XiangRongLin commented Jan 18, 2022

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Ensure order of search history entries in tests

Fixes the following issue(s)

Due diligence

❗ squash merge

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM, however the same problem might also happen here:

SearchHistoryEntry(OffsetDateTime.now().minusSeconds(7), 2, "AC"),

Could you fix it also there?
A constant would be great 😄

@XiangRongLin
Copy link
Collaborator Author

however the same problem might also happen here:

How would it also happen there? every line has the .minusSeconds(x)

@litetex
Copy link
Member

litetex commented Jan 19, 2022

@XiangRongLin

The problem here is that the value can't be controlled by the test.
OffsetDateTime.now() is not a constant, if there is e.g. a leap second or system clock update between the two lines in

SearchHistoryEntry(OffsetDateTime.now().minusSeconds(7), 2, "AC"),
SearchHistoryEntry(OffsetDateTime.now().minusSeconds(6), 0, "ABC"),
the test might fail.

I'm strongly in favor of implementing a hard-coded/current time independent timestamp like

OffsetDateTime.of(LocalDateTime.of(2000, 1, 1, 1, 1), ZoneOffset.UTC)

and then use that one for the tests.

@litetex
Copy link
Member

litetex commented Jan 20, 2022

There seems to be something wrong with the test on Emulator API 29:
https://github.com/TeamNewPipe/NewPipe/actions/runs/1724908322/attempts/2
https://github.com/TeamNewPipe/NewPipe/actions/runs/1724908322/attempts/1

org.schabi.newpipe.local.history.HistoryRecordManagerTest > onSearched[test(AVD) - 10] FAILED 
	org.junit.runners.model.TestTimedOutException: test timed out after 1 seconds
	at org.assertj.core.internal.Strings.instance(Strings.java:118)

not sure what's causing that but according to https://stackoverflow.com/questions/17016011/junit-test-times-out-despite-executing-quickly and
grafik
it has has be an InterruptedException inside

manager.onSearched(0, "Hello").test().await().assertValue(1)
// For some reason the Flowable returned by getAll() never completes, so we can't assert
// that the number of Lists it returns is exactly 1, we can only check if the first List is
// correct. Why on earth has a Flowable been used instead of a Single for getAll()?!?
val entities = database.searchHistoryDAO().all.blockingFirst()
assertThat(entities).hasSize(1)
assertThat(entities[0].id).isEqualTo(1)
assertThat(entities[0].serviceId).isEqualTo(0)
assertThat(entities[0].search).isEqualTo("Hello")

@sonarcloud
Copy link

sonarcloud bot commented Jan 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

How fast a tests is executed on a shared CI pipeline is not predictable as the build might be throttled because other builds are running.
Therefore adding extremely short timeouts inside the tests - where they can't be changed - is a bad idea.
Removed them for now.
@litetex
Copy link
Member

litetex commented Jan 21, 2022

The above error now also occurs on other branches, e.g. the dev-Branch.

Above error should be fixed with a6515d5.
There seems to be a performance problem with GH actions which causes the Android API 29 test do not run fast enough (needs more than 1s) on the VM which causes the above problems.

The internal was - for a unknown reason - changed from 10s to 1s inside 7d6688f#diff-52b3c5a209220512cb671827511b090b467afb544abcd94da776e7273072ffc0R24

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM

@litetex litetex added the bug Issue is related to a bug label Jan 21, 2022
@litetex litetex merged commit 81843dd into TeamNewPipe:dev Jan 21, 2022
@XiangRongLin XiangRongLin deleted the search_history_test branch January 22, 2022 05:56
This was referenced Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI pipeline maybe flaky (HistoryRecordManagerTest#deleteSearchHistory)
2 participants