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

Comments

dawehner created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2680057-2.patch1.56 KB

There we go

isntall’s picture

isntall 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
commented
StatusFileSize
new 2680057-4.patch2.07 KB
new 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
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
commented
Status: Needs work » Needs review
StatusFileSize
new 2680057-7.patch1.86 KB

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

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new interdiff.txt630 bytes
Mixologic’s picture

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

yay. This is looking good.

alexpott’s picture

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

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

dawehner’s picture

dawehner
Primary language German
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 🇪🇺🌍
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 🇪🇺🌍
commented
StatusFileSize
new 10-14-interdiff.txt1.01 KB
new 2680057-14.patch2.22 KB

@dawehner - yep it is prettier...

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Perfect

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 13-16-interdiff.txt562 bytes
new 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 transaction

isntall’s picture

isntall 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 Toronto, Canada
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
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
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 🇪🇺🌍
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 commented

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

dawehner’s picture

dawehner
Primary language German
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.