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

Add if-else statement to run tests against real website on schedule #516

Merged
merged 1 commit into from
Jan 16, 2021

Conversation

XiangRongLin
Copy link
Collaborator

  • I carefully read the contribution guidelines and agree to them.
  • I have tested the API against NewPipe.
  • I agree to create a pull request for NewPipe as soon as possible to make it compatible with the changed API.

I realized that running the tests against real website is basically just copying the ci.yml and changing one value.
Which can also be done with an if condition.

Because github actions if has no else, the shell script ones are used

@Stypox
Copy link
Member

Stypox commented Jan 15, 2021

I like this! Is it really that simple? :-D

@XiangRongLin
Copy link
Collaborator Author

XiangRongLin commented Jan 15, 2021

Should be.
I forgot to link this one: XiangRongLin#4
where the action does: https://github.com/XiangRongLin/NewPipeExtractor/pull/4/checks#step:2:8

@XiangRongLin
Copy link
Collaborator Author

@Stypox let the action on my repo run for a night and tomorrow we can see if it echoed the correct thing

@XiangRongLin
Copy link
Collaborator Author

Seeing how there already are new failing tests not even an hour after merging #512, we probably need a way to mark tests to be skipped in the ci pipeline if they are not run with mocks.

@Stypox
Copy link
Member

Stypox commented Jan 15, 2021

@XiangRongLin good idea

@XiangRongLin
Copy link
Collaborator Author

let the action on my repo run for a night and tomorrow we can see if it echoed the correct thing

It works
https://github.com/XiangRongLin/NewPipeExtractor/runs/1711997487?check_suite_focus=true#step:2:8

@Stypox
Copy link
Member

Stypox commented Jan 16, 2021

Great! @TobiGr

Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

Can be merged once review is implemented

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@TobiGr TobiGr merged commit beb7050 into TeamNewPipe:dev Jan 16, 2021
@TobiGr
Copy link
Member

TobiGr commented Jan 16, 2021

Ah wait. wasn't there a complaint regarding automated testing when they took down youtube-dl?

@TheAssassin This introduces sheduled tests against the real websites, whereas some predefined JSONs are tested against PRs. Is that ok?

@XiangRongLin XiangRongLin deleted the schedule_ci_with_real branch January 16, 2021 12:08
@XiangRongLin
Copy link
Collaborator Author

XiangRongLin commented Jan 17, 2021

@TobiGr I think part of it was that there we unit tests, that were downloading copyrighted stuff

see last paragraph of this section https://github.blog/2020-11-16-standing-up-for-developers-youtube-dl-is-back/#youtube-dl

So we would need to check all tests once and see if there are any such tests and replace them

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.

3 participants