The tests for Acquia Search are out of date and incompatible with D7's SimpleTest implementation. It would be great to revive the tests to ensure the module is working properly.

CommentFileSizeAuthor
#23 1784990-23.patch10.39 KBNick_vh
#19 1784990-19.patch10.42 KBNick_vh
#16 1784990-16.patch11.24 KBNick_vh
#13 1784990-12.patch10.67 KBNick_vh
#11 acquia-search-tests-1784990-11.patch9.5 KBcpliakas
#8 acquia-search-tests-1784990-8.patch9.53 KBcpliakas
#6 acquia-search-tests-1784990-6.patch9 KBcpliakas
#2 acquia-search-tests-1784990-2.patch7.57 KBcpliakas
#1 acquia-search-tests-1784990-1.patch4.95 KBcpliakas
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Active » Needs work
FileSize
acquia-search-tests-1784990-1.patch4.95 KB

First patch that adds tests for the derived key and HMAC generation.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs work » Needs review
FileSize
acquia-search-tests-1784990-2.patch7.57 KB

The attached patch is a good start and ready for review. It has two known failures pending the bugfixes for the following issues:

Clearly more tests are warranted, but it might be best to start small.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
Status: Needs review » Needs work

Applies cleanly and every test passes but the two mentioned.

Some nitpicks:

+++ b/acquia_search/tests/acquia_search.test
@@ -1,65 +1,130 @@
+class AcquiaSearchTest extends DrupalWebTestCase {

I'm under the impression that DrupalUnitTestCase is faster than WebTestCase, but I don't have data to back that up. Perhaps though since every test but EnvironmentUI is more of a unit test it'd be better to separate?

+++ b/acquia_search/tests/acquia_search.test
@@ -1,65 +1,130 @@
+    $url = 'http://search.acquia.com/solr/' . $this->id;

Is there a chance of this URL changing? If so it could be a property of the class in setUp() so it's more noticeable to change.

If you disagree feel free to move back to CNR.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Thanks for the review.

I'm under the impression that DrupalUnitTestCase is faster than WebTestCase

That seems right to me. I'll separate our the UI stuff and the low level functionality into the appropriate classes.

Is there a chance of this URL changing?

Yes, that's a good point. It seems there is some code in the middle of acquia_search_get_environment() that calculates the hostname. Maybe it would be good to separate out that code into an isolated function so that we could re-use it in setUp() as you mentioned.

Thanks again,
Chris

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

So I refactored the code using DrupalUnitTestCase, and it appears the I can use the testDerivedKey() and testHTMA() methods with that base class, but none of the others since they require connections to the database. Just moving those functions cut my test times from about 2 min to a little over 1 min, so that is pretty significant!

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
FileSize
acquia-search-tests-1784990-6.patch9 KB

I held off on breaking up the acquia_search_get_environment() to avoid too many changes at once, but we can attack that when we want to unit test that functionality. Attached patch has the suggestions in #1784990-3: Revive unit tests for Acquia Search.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs work » Needs review

Marking as "needs review" ...

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
FileSize
acquia-search-tests-1784990-8.patch9.53 KB

Added one more test (last one) which fails because of #1784804: Acquia Search is not set to the default environment on install but is important in assuring that the issue is fixed.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs review » Needs work

Hm, unit tests are now failing with a fatal error for some reason. Marking a needs work pending further investigation.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Ah. RTFM, Chris.

#887134: Setting up modules in DrupalUnitTestCase is useless

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs work » Needs review
FileSize
acquia-search-tests-1784990-11.patch9.5 KB

Let's try this again.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs review » Needs work

Too many typos :-(

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Needs work » Needs review
FileSize
1784990-12.patch10.67 KB

Let's see this one - I added some small tests for the hmac cookie, fixed a typo, added some comments and preventively I added some static clears

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs review » Reviewed & tested by the community

Looks good. I like the changes, and can confirm all tests pass.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented

Minor, can be changed before your commit.

+++ b/acquia_search/tests/acquia_search.test
@@ -1,66 +1,197 @@
+      'description' => 'Tests the Acquia Search user interface anf functionality.',

"and"

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Reviewed & tested by the community » Needs review
FileSize
1784990-16.patch11.24 KB
Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Needs review » Reviewed & tested by the community

marking it rtbc

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Project: Acquia Connector » Acquia Search
Version: 7.x-2.x-dev » 6.x-3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

And committed! Moving this to Acquia Search 6.x-3.x for backport

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Patch (to be ported) » Needs review
FileSize
1784990-19.patch10.42 KB

And a working one for Acquia Search 6.x-3.x

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

Re #18, Woo-hoo!

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Project: Acquia Search » Acquia Connector
Version: 6.x-3.x-dev » 7.x-2.x-dev

Doesn't look like this was committed to the 7.x-2.x branch yet.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Project: Acquia Connector » Acquia Search
Version: 7.x-2.x-dev » 6.x-3.x-dev

It was committed, just not pushed - Happens when you want to finish something before ending the day! :)

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
FileSize
1784990-23.patch10.39 KB

Improved it with a new line on the end of the test file.

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

for the randomAcquiaSearchName I could create a new class and let them inherit from that class, but I found that a little bit over the top.
I'm willing to change it to this architecture though. Just let me know how you'd like it.

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented
Status: Needs review » Reviewed & tested by the community

Not sure anything needs to change. The patch applies cleanly and the test pass for me, so marking as RTBC.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Does this work with ACQUIA_DEVELOPMENT_NOSSL removed?

cpliakas’s picture

cpliakas CreditAttribution: cpliakas commented

For 6.x-3.x, or in general?

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

It does not matter for the tests

ACQUIA_DEVELOPMENT_NOSSL is used in acquia_search_auth_cookie(), and the test does not use this (it uses acquia_search_authenticator())
ACQUIA_DEVELOPMENT_NOSSL is also used in AcquiaSearchService::prepareRequest and we also don't test this *yet* (used in makeServletRequest, sendRawGet and sendRawPost)

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Applied patch and ran it w/ success locally.

coltrane’s picture

coltrane
he/him
CreditAttribution: coltrane commented
Status: Reviewed & tested by the community » Needs work

ACQUIA_DEVELOPMENT_NOSSL is now defined in acquia_connector_test.module. It may need to move to acquia_agent.test if there are some test cases that don't include the test module, but that can happen in a followup.

#23 has incorrect line-endings. Is that a problem of the file or your editor Nick?

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented

Incorrect line endings?

Please clarify? I've never encountered any problems with that?

Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh commented
Status: Needs work » Fixed

Committed, Ben said the line endings were ok - we can revisit that in separate patch

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.