Skip to content

Commit 98a3a7d

Browse files
aljcalandraaljcalandra
authored
Merge pull request #147 from magento/MQE-1017
MQE-1017: Better Error Messaging When Non-Whitespace Characters Are Outside XML Elements
2 parents c66df37 + d06d9fb commit 98a3a7d

File tree

8 files changed

+172
-28
lines changed

8 files changed

+172
-28
lines changed

‎dev/tests/_bootstrap.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,8 @@
3131
\Magento\FunctionalTestingFramework\Config\MftfApplicationConfig::create(
3232
true,
3333
\Magento\FunctionalTestingFramework\Config\MftfApplicationConfig::GENERATION_PHASE,
34-
true
34+
true,
35+
false
3536
);
3637

3738
// Load needed framework env params

‎dev/tests/util/MftfTestCase.php‎

Lines changed: 62 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,20 @@
55
*/
66
namespace tests\util;
77

8+
use Magento\FunctionalTestingFramework\ObjectManager;
89
use Magento\FunctionalTestingFramework\Test\Handlers\TestObjectHandler;
910
use Magento\FunctionalTestingFramework\Util\TestGenerator;
1011
use PHPUnit\Framework\TestCase;
1112

1213
abstract class MftfTestCase extends TestCase
1314
{
14-
const RESOURCES_PATH = __DIR__ . '/../verification/Resources';
15+
const RESOURCES_PATH = __DIR__ .
16+
DIRECTORY_SEPARATOR .
17+
'..' .
18+
DIRECTORY_SEPARATOR .
19+
'verification' .
20+
DIRECTORY_SEPARATOR .
21+
'Resources';
1522

1623
/**
1724
* Private function which takes a test name, generates the test and compares with a correspondingly named txt file
@@ -37,4 +44,57 @@ public function generateAndCompareTest($testName)
3744
$cestFile
3845
);
3946
}
40-
}
47+
48+
/**
49+
* Private function which attempts to generate tests given an invalid shcema of a various type
50+
*
51+
* @param string[] $fileContents
52+
* @param string $objectType
53+
* @param string $expectedError
54+
* @throws \Exception
55+
*/
56+
public function validateSchemaErrorWithTest($fileContents, $objectType ,$expectedError)
57+
{
58+
$this->clearHandler();
59+
$fullTestModulePath = TESTS_MODULE_PATH .
60+
DIRECTORY_SEPARATOR .
61+
'TestModule' .
62+
DIRECTORY_SEPARATOR .
63+
$objectType .
64+
DIRECTORY_SEPARATOR;
65+
66+
foreach ($fileContents as $fileName => $fileContent) {
67+
$tempFile = $fullTestModulePath . $fileName;
68+
$handle = fopen($tempFile, 'w') or die('Cannot open file: ' . $tempFile);
69+
fwrite($handle, $fileContent);
70+
fclose($handle);
71+
}
72+
try {
73+
$this->expectExceptionMessage($expectedError);
74+
TestObjectHandler::getInstance()->getObject("someTest");
75+
} finally {
76+
foreach (array_keys($fileContents) as $fileName) {
77+
unlink($fullTestModulePath . $fileName);
78+
}
79+
$this->clearHandler();
80+
}
81+
}
82+
83+
/**
84+
* Clears test handler and object manager to force recollection of test data
85+
*
86+
* @throws \Exception
87+
*/
88+
private function clearHandler()
89+
{
90+
// clear test object handler to force recollection of test data
91+
$property = new \ReflectionProperty(TestObjectHandler::class, 'testObjectHandler');
92+
$property->setAccessible(true);
93+
$property->setValue(null);
94+
95+
// clear test object handler to force recollection of test data
96+
$property = new \ReflectionProperty(ObjectManager::class, 'instance');
97+
$property->setAccessible(true);
98+
$property->setValue(null);
99+
}
100+
}

‎dev/tests/verification/TestModule/Test/PageReplacementTest.xml‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,4 @@
2222
<amOnPage stepKey="oneParamAdminPageString" url="{{AdminOneParamPage.url('StringLiteral')}}"/>
2323
<amOnUrl stepKey="onExternalPage" url="{{ExternalPage.url}}"/>
2424
</test>
25-
<test name="ExternalPageTestBadReference">
26-
<amOnPage stepKey="onExternalPage" url="{{ExternalPage.url}}"/>
27-
</test>
2825
</tests>

‎dev/tests/verification/Tests/SchemaValidationTest.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
namespace tests\verification\Tests;
7+
8+
use Magento\FunctionalTestingFramework\Config\MftfApplicationConfig;
9+
use tests\util\MftfTestCase;
10+
use AspectMock\Test as AspectMock;
11+
12+
class SchemaValidationTest extends MftfTestCase
13+
{
14+
/**
15+
* Test generation of a test referencing an action group with no arguments
16+
*
17+
* @throws \Exception
18+
* @throws \Magento\FunctionalTestingFramework\Exceptions\TestReferenceException
19+
*/
20+
public function testInvalidTestSchema()
21+
{
22+
AspectMock::double(MftfApplicationConfig::class, ['debugEnabled' => true]);
23+
$testFile = ['testFile.xml' => "<tests><test name='testName'><annotations>a</annotations></test></tests>"];
24+
$expectedError = TESTS_MODULE_PATH .
25+
DIRECTORY_SEPARATOR .
26+
"TestModule" .
27+
DIRECTORY_SEPARATOR .
28+
"Test" .
29+
DIRECTORY_SEPARATOR .
30+
"testFile.xml";
31+
$this->validateSchemaErrorWithTest($testFile, 'Test', $expectedError);
32+
}
33+
34+
/**
35+
* After method functionality
36+
* @return void
37+
*/
38+
protected function tearDown()
39+
{
40+
AspectMock::clean();
41+
}
42+
}

‎src/Magento/FunctionalTestingFramework/Config/MftfApplicationConfig.php‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@ class MftfApplicationConfig
3434
*/
3535
private $verboseEnabled;
3636

37+
/**
38+
* Determines whether the user would like to execute mftf in a verbose run.
39+
*
40+
* @var bool
41+
*/
42+
private $debugEnabled;
43+
3744
/**
3845
* MftfApplicationConfig Singelton Instance
3946
*
@@ -47,10 +54,15 @@ class MftfApplicationConfig
4754
* @param bool $forceGenerate
4855
* @param string $phase
4956
* @param bool $verboseEnabled
57+
* @param bool $debugEnabled
5058
* @throws TestFrameworkException
5159
*/
52-
private function __construct($forceGenerate = false, $phase = self::EXECUTION_PHASE, $verboseEnabled = null)
53-
{
60+
private function __construct(
61+
$forceGenerate = false,
62+
$phase = self::EXECUTION_PHASE,
63+
$verboseEnabled = null,
64+
$debugEnabled = null
65+
) {
5466
$this->forceGenerate = $forceGenerate;
5567

5668
if (!in_array($phase, self::MFTF_PHASES)) {
@@ -59,6 +71,7 @@ private function __construct($forceGenerate = false, $phase = self::EXECUTION_PH
5971

6072
$this->phase = $phase;
6173
$this->verboseEnabled = $verboseEnabled;
74+
$this->debugEnabled = $debugEnabled;
6275
}
6376

6477
/**
@@ -68,12 +81,14 @@ private function __construct($forceGenerate = false, $phase = self::EXECUTION_PH
6881
* @param bool $forceGenerate
6982
* @param string $phase
7083
* @param bool $verboseEnabled
84+
* @param bool $debugEnabled
7185
* @return void
7286
*/
73-
public static function create($forceGenerate, $phase, $verboseEnabled)
87+
public static function create($forceGenerate, $phase, $verboseEnabled, $debugEnabled)
7488
{
7589
if (self::$MFTF_APPLICATION_CONTEXT == null) {
76-
self::$MFTF_APPLICATION_CONTEXT = new MftfApplicationConfig($forceGenerate, $phase, $verboseEnabled);
90+
self::$MFTF_APPLICATION_CONTEXT =
91+
new MftfApplicationConfig($forceGenerate, $phase, $verboseEnabled, $debugEnabled);
7792
}
7893
}
7994

@@ -115,6 +130,17 @@ public function verboseEnabled()
115130
return $this->verboseEnabled ?? getenv('MFTF_DEBUG');
116131
}
117132

133+
/**
134+
* Returns a boolean indicating whether the user has indicated a debug run, which will lengthy validation
135+
* with some extra error messaging to be run
136+
*
137+
* @return bool
138+
*/
139+
public function debugEnabled()
140+
{
141+
return $this->debugEnabled ?? getenv('MFTF_DEBUG');
142+
}
143+
118144
/**
119145
* Returns a string which indicates the phase of mftf execution.
120146
*

‎src/Magento/FunctionalTestingFramework/Config/Reader/Filesystem.php‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -157,17 +157,14 @@ protected function readFiles($fileList)
157157
} else {
158158
$configMerger->merge($content);
159159
}
160+
if (MftfApplicationConfig::getConfig()->debugEnabled()) {
161+
$this->validateSchema($configMerger, $fileList->getFilename());
162+
}
160163
} catch (\Magento\FunctionalTestingFramework\Config\Dom\ValidationException $e) {
161164
throw new \Exception("Invalid XML in file " . $fileList->getFilename() . ":\n" . $e->getMessage());
162165
}
163166
}
164-
if ($this->validationState->isValidationRequired()) {
165-
$errors = [];
166-
if ($configMerger && !$configMerger->validate($this->schemaFile, $errors)) {
167-
$message = "Invalid Document \n";
168-
throw new \Exception($message . implode("\n", $errors));
169-
}
170-
}
167+
$this->validateSchema($configMerger);
171168

172169
$output = [];
173170
if ($configMerger) {
@@ -220,4 +217,23 @@ protected function verifyFileEmpty($content, $fileName)
220217
}
221218
return true;
222219
}
220+
221+
/**
222+
* Validate read xml against expected schema
223+
*
224+
* @param string $configMerger
225+
* @param string $filename
226+
* @throws \Exception
227+
* @return void
228+
*/
229+
protected function validateSchema($configMerger, $filename = null)
230+
{
231+
if ($this->validationState->isValidationRequired()) {
232+
$errors = [];
233+
if ($configMerger && !$configMerger->validate($this->schemaFile, $errors)) {
234+
$message = $filename ? $filename . PHP_EOL . "Invalid Document \n" : PHP_EOL . "Invalid Document \n";
235+
throw new \Exception($message . implode("\n", $errors));
236+
}
237+
}
238+
}
223239
}

‎src/Magento/FunctionalTestingFramework/Config/Reader/MftfFilesystem.php‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
namespace Magento\FunctionalTestingFramework\Config\Reader;
88

9+
use Magento\FunctionalTestingFramework\Config\MftfApplicationConfig;
910
use Magento\FunctionalTestingFramework\Exceptions\Collector\ExceptionCollector;
1011
use Magento\FunctionalTestingFramework\Util\Iterator\File;
1112

@@ -21,7 +22,6 @@ class MftfFilesystem extends \Magento\FunctionalTestingFramework\Config\Reader\F
2122
public function readFiles($fileList)
2223
{
2324
$exceptionCollector = new ExceptionCollector();
24-
$errors = [];
2525
/** @var \Magento\FunctionalTestingFramework\Test\Config\Dom $configMerger */
2626
$configMerger = null;
2727
foreach ($fileList as $key => $content) {
@@ -40,17 +40,15 @@ public function readFiles($fileList)
4040
} else {
4141
$configMerger->merge($content, $fileList->getFilename(), $exceptionCollector);
4242
}
43+
if (MftfApplicationConfig::getConfig()->debugEnabled()) {
44+
$this->validateSchema($configMerger, $fileList->getFilename());
45+
}
4346
} catch (\Magento\FunctionalTestingFramework\Config\Dom\ValidationException $e) {
4447
throw new \Exception("Invalid XML in file " . $key . ":\n" . $e->getMessage());
4548
}
4649
}
4750
$exceptionCollector->throwException();
48-
if ($this->validationState->isValidationRequired()) {
49-
if ($configMerger && !$configMerger->validate($this->schemaFile, $errors)) {
50-
$message = "Invalid Document \n";
51-
throw new \Exception($message . implode("\n", $errors));
52-
}
53-
}
51+
$this->validateSchema($configMerger, $fileList->getFilename());
5452

5553
$output = [];
5654
if ($configMerger) {

‎src/Magento/FunctionalTestingFramework/Console/GenerateTestsCommand.php‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ protected function configure()
3636
->addOption("config", 'c', InputOption::VALUE_REQUIRED, 'default, singleRun, or parallel', 'default')
3737
->addOption("force", 'f',InputOption::VALUE_NONE, 'force generation of tests regardless of Magento Instance Configuration')
3838
->addOption('time', 'i', InputOption::VALUE_REQUIRED, 'Used in combination with a parallel configuration, determines desired group size (in minutes)', 10)
39-
->addOption('tests', 't', InputOption::VALUE_REQUIRED, 'A parameter accepting a JSON string used to determine the test configuration');
39+
->addOption('tests', 't', InputOption::VALUE_REQUIRED, 'A parameter accepting a JSON string used to determine the test configuration')
40+
->addOption('debug', 'd', InputOption::VALUE_NONE, 'run extra validation when generating tests');
4041
}
4142

4243
/**
@@ -54,6 +55,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
5455
$json = $input->getOption('tests');
5556
$force = $input->getOption('force');
5657
$time = $input->getOption('time') * 60 * 1000; // convert from minutes to milliseconds
58+
$debug = $input->getOption('debug');
5759
$verbose = $output->isVerbose();
5860

5961
if ($json !== null && !json_decode($json)) {
@@ -66,7 +68,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
6668
throw new TestFrameworkException("time option cannot be less than or equal to 0");
6769
}
6870

69-
$testConfiguration = $this->createTestConfiguration($json, $tests, $force, $verbose);
71+
$testConfiguration = $this->createTestConfiguration($json, $tests, $force, $debug, $verbose);
7072

7173
// create our manifest file here
7274
$testManifest = TestManifestFactory::makeManifest($config, $testConfiguration['suites']);
@@ -92,16 +94,18 @@ protected function execute(InputInterface $input, OutputInterface $output)
9294
* @param string $json
9395
* @param array $tests
9496
* @param bool $force
97+
* @param bool $debug
9598
* @param bool $verbose
9699
* @return array
97100
*/
98-
private function createTestConfiguration($json, array $tests, bool $force, bool $verbose)
101+
private function createTestConfiguration($json, array $tests, bool $force, bool $debug, bool $verbose)
99102
{
100103
// set our application configuration so we can references the user options in our framework
101104
MftfApplicationConfig::create(
102105
$force,
103106
MftfApplicationConfig::GENERATION_PHASE,
104-
$verbose
107+
$verbose,
108+
$debug
105109
);
106110

107111
$testConfiguration = [];

0 commit comments

Comments
 (0)