Comments

sabina.h created an issue. See original summary.

bcizej’s picture

sabina.h’s picture

Status: Active » Needs review
StatusFileSize
new3.7 KB

Providing a patch for this issue.

bcizej’s picture

Status: Needs review » Needs work

Nice work thanks @sabina.h

Couple of minor issues to correct here:

/**
   * @var array
   */
  public static $modules = [
    'node',
    'user',
    'autologout',
  ];

You can use

 /**
   * {@inheritdoc}
   */ 

for this variable.

/**
   * Performs any pre-requisite tasks that need to happen.
   */
  protected function setUp() {

Here as well.

/**
   * Test that configurable text is displayed in the popup.
   */
  public function testConfigurableText() {
/**
   * Test that disable buttons settings option works as expected.
   */

Method summary should be in 3rd person. https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
Tests that configurable text is displayed in the popup.

$this->moduleConfig->set('timeout', 10)
      ->set('padding', 10)
      ->save();
sleep(15);

The timeout and sleep values could be lowered.

drupal_flush_all_caches();
$this->drupalGet('node');

If autologout settings are not updated because of caching then this is a problem that needs to be further investigated and find a fix.

sabina.h’s picture

Status: Needs work » Needs review
StatusFileSize
new3.69 KB
new2.61 KB

Thank you for you review. Providing the corrected patch with the interdiff. I have also tested the autologout settings and came to a conclusion that there aren't any cache issues.

deaom’s picture

Status: Needs review » Needs work

Hi, I tested the provided patch and it applies and also tests go through on my local machine. If I understood @bcizej correctly, he also suggested to remove the drupal_flush_all_caches(); all other suggestions are added to the patch. I tested it without that line and the tests are still working. So marking this as Needs work, for that line to be removed, everything else looks good to me.
When uploading the interdiff file either choose the do not test option or upload with the .txt extension, so the file does not get tested :).

sabina.h’s picture

Status: Needs work » Needs review
StatusFileSize
new570 bytes
new3.61 KB

Thank you @DeaOm for your review. Here is the corrected patch.

Status: Needs review » Needs work

The last submitted patch, 7: 3185506-7.patch, failed testing. View results

bcizej’s picture

Tests are failing because of #3190324: Settings are not updated on cached pages

So we have to wait for that issue to get merged before testing again.

bcizej’s picture

Title: Add automatic tests » Add automatic tests for modal window configuration
Issue summary: View changes
bcizej’s picture

Title: Add automatic tests for modal window configuration » Add automatic tests for confirmation modal config
bcizej’s picture

StatusFileSize
new3.89 KB
new3.37 KB

Meanwhile I cleaned the code a bit.

Removed unneeded node module in test and since this is a Functional Javascript test we can use

$this->assertSession()->waitForElement();

instead of

sleep()

Also we don't need to assert if user is logged in so I removed those assertions.
$this->drupalLogin() already asserts that though.

bcizej’s picture

Status: Needs work » Needs review

#3190324: Settings are not updated on cached pages has been commited so this is ready for review.

deaom’s picture

Status: Needs review » Reviewed & tested by the community

Tested the patch on the newest dev version of the module (which has the cache correction) locally on Drupal core 8.9.11 and test are also passing. The added changes did not break anything, so everything seems fine from my side. Marking this as RTBC.

japerry’s picture

Added to 1.4 release issue #3293639: Issues for 8.x-1.4 release

japerry’s picture

Status: Reviewed & tested by the community » Fixed

  • japerry committed 4d14dbb on 8.x-1.x authored by bcizej
    Issue #3185506: Add automatic tests for confirmation modal config
    

Status: Fixed » Closed (fixed)

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