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

Update vw_adlist (tests) #1208

Merged
merged 1 commit into from
Oct 17, 2021
Merged

Update vw_adlist (tests) #1208

merged 1 commit into from
Oct 17, 2021

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Oct 7, 2021

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?: {replace this text with a number from 1 to 10, with 1 being not familiar, and 10 being very familiar}
5


pi-hole/pi-hole#4372 removed vw_adlist from the gravity.db. There is no need anymore to test in this repo if the view does not exist anymore.

Changes the test for vw_adlist to fit https://github.com/pi-hole/pi-hole/pull/4379/files

DL6ER
DL6ER previously requested changes Oct 8, 2021
Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

--

@DL6ER
Copy link
Member

DL6ER commented Oct 8, 2021

The CircleCI tests failed because of the non-existing parent tag remove (branch names are translated to directories on https://ftl.pi-hole.net). I added this now and restarted the tests.

I also restarted the GHA tests. DNS tests fail here a lot more often than on CircleCI. Restarted them as well.

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this despite the failing GHA tests (the Circle tests pass fine), however, see my comment about dropping vs. changing vw_adlist on the core PR.

@yubiuser
Copy link
Member Author

yubiuser commented Oct 8, 2021

I adjusted the test to fit https://github.com/pi-hole/pi-hole/pull/4379/files

@yubiuser yubiuser added the PR: Approval Required Open Pull Request, needs approval label Oct 9, 2021
@DL6ER
Copy link
Member

DL6ER commented Oct 9, 2021

I will try to get this (and the accompanying core PR) tested tomorrow. Depends on how much time I'll have after reworking the tests so that they stop having issues on GHA (maybe GHA servers are not meant to connect to the internet or, at least, not often)

DL6ER
DL6ER previously approved these changes Oct 10, 2021
@DL6ER
Copy link
Member

DL6ER commented Oct 10, 2021

#1212 is up to fix the tests on GHA

@DL6ER
Copy link
Member

DL6ER commented Oct 10, 2021

If you could rebase on (or merge) development, the tests should be all green.

@PromoFaux
Copy link
Member

one more rebase on development please, @yubiuser

@PromoFaux PromoFaux force-pushed the remove/vw_adlist branch 3 times, most recently from 0df7309 to e35576c Compare October 12, 2021 22:37
Signed-off-by: yubiuser <[email protected]>
@DL6ER DL6ER changed the title Remove vw_adlist from test suit Update vw_adlist (tests) Oct 17, 2021
@DL6ER DL6ER merged commit 9be7bcd into development Oct 17, 2021
@DL6ER DL6ER deleted the remove/vw_adlist branch October 17, 2021 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approval Required Open Pull Request, needs approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants