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

feature: Added phpunit db setup tests #5594

Merged
merged 11 commits into from Feb 7, 2017

Conversation

Projects
None yet
4 participants
@laf
Member

laf commented Jan 25, 2017

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

I'm sure @murrant will rewrite this but it gets us validation at the moment that build-base will work

Doesn't use mysql strict at the moment as it will fail with this.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 25, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@laf

This comment has been minimized.

Member

laf commented Jan 25, 2017

Not sure why hhvm is failing here, especially seeing as it's set to allow failures.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 31, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@laf

This comment has been minimized.

Member

laf commented Jan 31, 2017

@librenms/reviewers Rebased and it's now all working, think murrant fixed something :)

I have tested php build-base.php and web installs still work off the back of this.

@@ -32,8 +32,10 @@
chdir($install_dir);
// Libraries
require('Net/IPv4.php');
require('Net/IPv6.php');
if (!module_selected('phpunit', $init_modules)) {

This comment has been minimized.

@murrant

murrant Feb 3, 2017

Member

why are these in a if statement?

This comment has been minimized.

@laf

laf Feb 3, 2017

Member

I couldn't install them into the travis env so the test would fail.

This comment has been minimized.

@murrant

murrant Feb 3, 2017

Member

Odd, I don't remember running into that problem...

This comment has been minimized.

@laf

laf Feb 3, 2017

Member

How did you do it before? We ask users to install them via pear or apt-get normally.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Feb 3, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Feb 3, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Feb 3, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Feb 4, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@laf

This comment has been minimized.

Member

laf commented Feb 4, 2017

Reverted back to not using net/ipvX modules.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Feb 7, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

Rename the file to the correct name.
Get rid of the warning in build-base.php
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Feb 7, 2017

Auto-Deploy finished, Test PR at http://5594.ci.librenms.org or https://5594.ci.librenms.org

@murrant

murrant approved these changes Feb 7, 2017

Hope you didn't intend the DBSetup test to run first.

Good to go.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Feb 7, 2017

The inspection completed: 1 new issues, 4 updated code elements

@laf

This comment has been minimized.

Member

laf commented Feb 7, 2017

Yes I put the do test first so it's ready for other tests. What's up with it being first?

@murrant

This comment has been minimized.

Member

murrant commented Feb 7, 2017

I changed around some things, so it isn't done first. I can put it back. But right now, the DBSetupTest creates a db, builds it up and then drops the database, so it is self-contained and no other tests rely on it.

If you want to build up a database for other tests to use, I don't think this is the right approach.

@laf

This comment has been minimized.

Member

laf commented Feb 7, 2017

I know overall it's not the right approach but we don't have a proper dbal to make use of things like in v2 so at present this is just a make do with what we have.

@murrant

This comment has been minimized.

Member

murrant commented Feb 7, 2017

I'm happy with this PR as a way to test our sql schema files. I think we should merge this as is and make changes later when we need the DB for another test.

@laf laf merged commit 200b762 into librenms:master Feb 7, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:phpunit-dbtest branch Feb 7, 2017

@laf

This comment has been minimized.

Member

laf commented Feb 7, 2017

I have started to look at using a dbal and writing wrappers around our current calls to the dbal. Probably the easiest way to deal with this for now.

@murrant

This comment has been minimized.

Member

murrant commented Feb 7, 2017

I looked at that awhile ago, then I ended up at a spot where I wanted full eloquent :(

Then, I decided it was better to focus energy elsewhere, like v2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment