Problem/Motivation

In order to support two kind of jobs (one with javascript tests, one without), the plan is to execute run-tests.sh twice in a row with different parameters

To make the result parsing as easy as possible, the testbot currently uses the sqlite database. Sadly by default run-tests.sh cleans up the existing one

Proposed resolution

Add a boolean flag to keep the results table.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 2680057-16.patch2.23 KBalexpott
#16 13-16-interdiff.txt562 bytesalexpott
#13 2680057-14.patch2.22 KBalexpott
#13 10-14-interdiff.txt1.01 KBalexpott
#10 2680057-10.patch1.77 KBalexpott
#10 7-10-interdiff.txt1.07 KBalexpott
#8 interdiff.txt630 bytesdawehner
#7 2680057-7.patch1.86 KBdawehner
#4 interdiff.txt1.2 KBdawehner
#4 2680057-4.patch2.07 KBdawehner
#2 2680057-2.patch1.56 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Active » Needs review
FileSize
2680057-2.patch1.56 KB

There we go

isntall’s picture

isntall CreditAttribution: isntall at Drupal Association commented

In my manual testing this patch works.
Though if there is no existing sqlite db, an error is thrown
ERROR: Missing Simpletest database schema. Either install Simpletest module or use the --sqlite parameter.
even though the --sqlite flag is present

This might not be an issue, but the error is miss leading.

The command i used to run the test.
cd /var/www/html && sudo -u www-data /opt/phpenv/shims/php /var/www/html/core/scripts/run-tests.sh --url http://localhost/checkout --dburl mysql://drupaltestbot:drupaltestbotpw@drupaltestbot-db-mysql-5-5/simpletest_1457380647 --color --keep-results --concurrency 1 --keep-results-table --sqlite /var/www/html/results/simpletest.sqlite --types PHPUnit-FunctionalJavascript --all

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2680057-4.patch2.07 KB
interdiff.txt1.2 KB

Good point. What about this then?

Status: Needs review » Needs work

The last submitted patch, 4: 2680057-4.patch, failed testing.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented 
+++ b/core/scripts/run-tests.sh
@@ -534,14 +541,21 @@ function simpletest_script_setup_database($new = FALSE) {
+  if ($new && $sqlite && ) {

Something fishy going on here

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs work » Needs review
FileSize
2680057-7.patch1.86 KB

At was actually just 11pm, one is getting old.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
interdiff.txt630 bytes
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented
Status: Needs review » Reviewed & tested by the community

yay. This is looking good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs review
FileSize
7-10-interdiff.txt1.07 KB
2680057-10.patch1.77 KB

We can achieve the same with slightly less duplication and complexity...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented 
+++ b/core/scripts/run-tests.sh
@@ -556,16 +556,13 @@ function simpletest_script_setup_database($new = FALSE) {
+          $schema->dropTable($name);
+          $table_exists = FALSE;

As said on IRC, we could truncate() the table and then not set $table_exists = FALSE

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@Mixologic pointed out that @isntall had also worked on a version of the patch but @dawehner was just quicker... ticking the credit box for @isntall.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
10-14-interdiff.txt1.01 KB
2680057-14.patch2.22 KB

@dawehner - yep it is prettier...

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Perfect

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/scripts/run-tests.sh
@@ -549,10 +557,13 @@ function simpletest_script_setup_database($new = FALSE) {
+          $connection->truncate($name);

Missing ->execute() :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
13-16-interdiff.txt562 bytes
2680057-16.patch2.23 KB

Fixed and started some db testing of truncate over in #2684359: The truncate method is does different things on different databases and depending if in a transation

isntall’s picture

isntall CreditAttribution: isntall at Drupal Association commented
Status: Needs review » Reviewed & tested by the community

This looks good, and behaves correctly.

My concern from comment #3 have been addressed.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Patch looks fine just on nit.

+++ b/core/scripts/run-tests.sh
@@ -549,10 +557,13 @@ function simpletest_script_setup_database($new = FALSE) {
+        $table_exists = $schema->tableExists($name);
+        if (empty($args['keep-results-table']) && $table_exists) {

Can we add a comment above this line to explain the situation?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

IMHO the code is pretty selfdescriptive already. If we don't want to keep the table and the table exists, we truncate it.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
CreditAttribution: Mixologic at Drupal Association commented

FYI: this is still a blocker to being able to commit the changes to drupalci.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Given that this is for non-runtime code and for DrupalCI and optional too boot I'm happy to commit this to unblock frontend testing on DrupalCI. Committed 5e42677 and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!

isntall’s picture

isntall CreditAttribution: isntall at Drupal Association commented

The new DCI tag has been deployed that takes advantage of this new functionality.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

You can see the change in git, but the issue doesn't have a comment mentioning it, weird.

  • alexpott committed 7e01fa5 on 8.2.x 
    Issue #2680057 by alexpott, dawehner, isntall: Allow to not override the...

  • alexpott committed 2e5142f on 8.1.x 
    Issue #2680057 by alexpott, dawehner, isntall: Allow to not override the...

  • alexpott committed 5e42677 on 8.0.x 
    Issue #2680057 by alexpott, dawehner, isntall: Allow to not override the...

Status: Fixed » Closed (fixed)

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