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

Testing: Blast Database Node #35

Merged
merged 9 commits into from
Jun 6, 2018
Merged

Testing: Blast Database Node #35

merged 9 commits into from
Jun 6, 2018

Conversation

laceysanderson
Copy link
Member

This is my first PR adding testing to this module 🎉

Specifically, this PR adds very basic testing for the blast database node. My intent was to simply begin (something is better then nothing) so it is not exhaustive at this point ;-p.

  • Add the framework for PHPUnit testing with the help of the Tripal Test Suite.
  • Add a Blast Database Node Database Seeder: tests/DatabaseSeeders/BlastDBNodeSeeder.php
    - Can be used in tests to quickly add a blast database node for testing purposes as seen in the update test.
  • Add basic exists, create and update for the blastdb node type: tests/BlastDBNodeTest.php

How to test locally:

  1. Navigate to your blast repo ( eg: DRUPAL_ROOT/sites/all/modules/tripal_blast)
  2. Install PHPUnit locally: composer install
  3. Describe your environment by copying test/example.env to test/.env, and DRUPAL_ROOT to match your drupal site.
  4. Run the tests: ./vendor/bin/phpunit Below is what the output looks like for me :-)

PHPUnit 5.7.27 by Sebastian Bergmann and contributors.

Runtime: PHP 5.6.7-1
Configuration: ./phpunit.xml

... 3 / 3 (100%)

Time: 1.12 seconds, Memory: 50.25MB

OK (3 tests, 6 assertions)

Notes

  • I've enabled Travis-CI for Tripal Blast so you should see if the tests passed or not on this PR without having to test locally :fingers crossed:

Guidance

I would love input on where to go from here! :-) Should I

  1. continue on with more complete testing of the blastdb node (used as the template in the blast form)
  2. test each API function (api/blast_ui.api.inc) independently
  3. focus on the blast form everyone uses ;-p

…ithub.com/statonlab/TripalTestSuite).

Specifically, I ran:
1. `composer require statonlab/tripal-test-suite --dev`
2. `./vendor/bin/tripaltest init tripal_blast`
3. Added to my .gitignore as suggested by #2.
That's All! Look how easy TripalTestSuite makes it to get started!
// Get a list of all types available.
$types = node_type_get_types();

// The BlastDB node type must be in the list.
Copy link
Member

Choose a reason for hiding this comment

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

You can pass the assertion a third option, which is an additional message to display if the test fails.

You might consider moving your comments into assertion fail messages: that way if the test fails, the dev will know why at the console without having to read the test documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

That’s awesome! I will definitely do that!

// Fill in the form.
$form_state = array(
'values' => array(
'db_name' => 'Test Blast Database',
Copy link
Member

@bradfordcondon bradfordcondon Jun 6, 2018

Choose a reason for hiding this comment

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

So if i run this test on my site with a pre-existing blast db that is Test Blast Database, what happens? If it lets me create it, your assertion that num results = 1 is false. If it wont let me create it, you'll get an error here.

What I do is use really stupid names in my tests (Banana Ostrich Blast Test) to reduce chance of these conflicts. Test Blast Database is actually the sort of test i might make when trying to set up the module the first time...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

@almasaeed2010 almasaeed2010 Jun 6, 2018

Choose a reason for hiding this comment

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

You can also use Faker to generate random strings :)

You can do that by adding this right under your namespace

use Faker\Factory;

And then using it as such:

$faker = Factory::create();

$name = $faker->name;
$someString = $faker->string;

If you'd like to know all available faker methods see: https://github.com/fzaninotto/Faker

EDIT: forgot to point out that this comes with TripalTestSuite by default so no need to install anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I will definitely use this for the create!

I would love to do this for the update too but since the blastdb is created in a dataseeder how would I know what to select in the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since test functions should be independent from each other (unless explicitly specified using PHPUnit doc blocks), you should actually create the and then update in the same function to test the update functionality. It's a bit repetitive but apparently that's the practice.

If you run the create in the seeder, you can always add a method to the seeder to return the created object (which you have saved into a private property) since it's just a PHP class :)

Copy link
Member

@bradfordcondon bradfordcondon left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks a lot for checking it out.

I made a couple of inline comments but in general your tests look pretty smart to me.

@laceysanderson
Copy link
Member Author

Implemented the awesome suggestions by @bradfordcondon and @almasaeed2010 :-)

@ekcannon & @bradfordcondon do you have any input on what I should test next?

I would love input on where to go from here! :-) Should I
- continue on with more complete testing of the blastdb node (used as the template in the blast form)
- test each API function (api/blast_ui.api.inc) independently
- focus on the blast form everyone uses ;-p

@bradfordcondon
Copy link
Member

What would be cool is if you installed blast on the travis environment and somehow thought of a smart way to loop through all the option combos presented in the blastn form and submitted a blast job with them, ensuring no errors. That has some tricky things though (some options are AJAX-dependent).

Otherwise try to focus on the "end points" of the module. So API functions that are true API functions (we expect them to be called by anything else) are great places to start.

@laceysanderson
Copy link
Member Author

That would be amazing! and ambitious ;-p but so worth it. I was tempted to start with the form as well. We'll see what I can figure out -I love a challenge after all ⚔️

@laceysanderson laceysanderson mentioned this pull request Jun 6, 2018
@laceysanderson
Copy link
Member Author

Moving discussion into #38 so I can merge these tests :-D 🎉

@laceysanderson laceysanderson merged commit 24a8d84 into 7.x-1.x Jun 6, 2018
@laceysanderson laceysanderson deleted the add-tests branch June 6, 2018 20:29
@laceysanderson laceysanderson restored the add-tests branch June 6, 2018 20:29
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.

None yet

3 participants