Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT created an issue. See original summary.

JeroenT’s picture

Status: Active » Needs review
FileSize
9.74 KB

Started working on converting the simpletests to PHPUnit tests. This still needs a lot of work.

Status: Needs review » Needs work

The last submitted patch, 2: 3061563-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Berdir’s picture

zeuty’s picture

zeuty’s picture

Assigned: Unassigned » zeuty
Status: Needs work » Needs review

Converted the tests.
Please review.

Berdir’s picture

Status: Needs review » Needs work

Great work on this.

  1. +++ b/tests/src/FunctionalJavascript/PathautoLocaleTest.php
    @@ -113,35 +114,47 @@ class PathautoLocaleTest extends WebTestBase {
    -    ];
    -    $this->drupalPostAjaxForm(NULL, $edit, 'type');
    -    $edit += [
           'pattern' => '/the-articles/[node:title]',
           'label' => 'English articles',
    -      'id' => 'english_articles',
           'bundles[article]' => TRUE,
           'languages[en]' => TRUE,
         ];
         $this->drupalPostForm(NULL, $edit, 'Save');
    -    $this->assertText('Pattern English articles saved.');
    +
    +    $edit += [
    +      'id' => 'english_articles',
    +    ];
    +    $this->drupalPostForm(NULL, $edit, 'Save');
    +    $this->assertSession()->pageTextContains('Pattern English articles saved.');
     
         // Create a pattern for French articles.
    

    I think this implicitly relies on it being too slow to do the machine-name conversion automatically and then fills out the now-visible field. That seems tricky and could possible result in random fails.

    We had the same in paragraphs, there we did it like this:

    -    static::fieldUIAddNewField('admin/structure/paragraphs_type/' . $paragraph_type, 'text', 'Text', 'text_long', [], []);
    +    $this->drupalGet('admin/structure/paragraphs_type/' . $paragraph_type . '/fields/add-field');
    +    $page->selectFieldOption('new_storage_type', 'text_long');
    +    $page->fillField('label', 'Text');
    +    $this->assertSession()->waitForElementVisible('css', '#edit-name-machine-name-suffix .link');
    +    $page->pressButton('Edit');
    +    $page->fillField('field_name', 'text');
    +    $page->pressButton('Save and continue');
    

    We should be able to do something similar here too?

  2. +++ b/tests/src/FunctionalJavascript/PathautoUiTest.php
    @@ -53,72 +54,64 @@ class PathautoUiTest extends WebTestBase {
         $this->drupalPostForm('admin/config/search/path/settings', $edit, 'Save configuration');
    -    /*$this->assertText('The field Maximum alias length cannot be less than 1.');
    -    $this->assertText('The field Maximum component length cannot be less than 1.');*/
    -    $this->assertNoText('The configuration options have been saved.');
    +    $this->assertSession()->responseNotContains('The configuration options have been saved.');
     
    

    wondering why the old assertions were commented out?

    Is it because we rely on a number field that ensures that with client-side validation?

    not sure we need to keep that test at all then, this just doesn't seem very useful.

    If anything, we could maybe assert that the specific field we're look for exists and has a min="1" attribute?

zeuty’s picture

FileSize
28.5 KB

@Berdir, thanks for review, I'll continue working on this.
Regarding the commented out strings - they were commented by you in 2014 :)
I just removed the commented our strings from the code.

Will update the patch soon.

MegaChriz’s picture

Note that #2756703: URL Alias not saving in some cases partial converts the test PathautoNodeWebTest::testCustomAliasWithoutPattern to phpunit.

zeuty’s picture

Adjusted according to remarks from @berdir in #7

zeuty’s picture

Status: Needs work » Needs review
zeuty’s picture

Status: Needs review » Needs work

Sorry, wrong patchfile

zeuty’s picture

Status: Needs work » Needs review
FileSize
25.75 KB
4.06 KB

  • Berdir committed 72fb423 on 8.x-1.x authored by zeuty
    Issue #3061563 by zeuty: Convert simpletest to PHPUnit tests
    
Berdir’s picture

Status: Needs review » Fixed

Thanks!

MahtabAlam’s picture

Status: Fixed » Needs review
FileSize
728 bytes

Call to deprecated method strtolower() will be also removed before Drupal 9.0.0. Use mb_strtolower() instead.

shubham.prakash’s picture

Assigned: zeuty » Unassigned
FileSize
728 bytes

RE: Call to deprecated method strtolower() will be also removed before Drupal 9.0.0. Use mb_strtolower() instead.
Hope this patch fixes the issue.

MahtabAlam’s picture

@shubham what is the difference between the patch which I provided and yours?

shubham.prakash’s picture

@MahtabAlam the patch in #16 given by you was displaying "Checkout Error" earlier, so i put my patch for the same issue. Now that, it is showing "46 pass", there is no need for patch in #17.

shubham.prakash’s picture

Also, this patch can be applied which fixes all these deprecated methods - strlen(), strtolower(), substr(), strpos().

Berdir’s picture

Status: Needs review » Fixed

This issue was about tests and has been committed, don't reopen it for other changes.

AFAIK there are existing issues for the mbstring functions.

Status: Fixed » Closed (fixed)

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

penyaskito’s picture

Not sure if this was on purpose, but maybe the Trait should have been deprecated AND moved instead of moved.

#3087523: PathautoTestHelperTrait namespace has changed

Sadly my dependence was on * instead of -dev and I didn't see this before the release :-(

Berdir’s picture

Sorry about that. Unlike core, I don't really consider our test classes and traits to be an API. Note that depending on a trait or base class for a dev-dependency only is actually pretty problematic as it can AFAIK still kill test disccovery when you try to run tests and that dependency isn't around.