Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

AF-1917: Make over_react_codemod script idempotent #5

Merged
merged 5 commits into from
Sep 24, 2018
Merged

AF-1917: Make over_react_codemod script idempotent #5

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
02fa070
AF-1917: make migrater script idempotent and add tests
sebastianmalysa-wf Aug 28, 2018
c879539
Merge branch 'master' of github.com:Workiva/over_react_codemod into A…
sebastianmalysa-wf Sep 19, 2018
21c3f07
AF-1917: format
sebastianmalysa-wf Sep 19, 2018
454fe99
Merge branch 'master' of github.com:Workiva/over_react_codemod into A…
sebastianmalysa-wf Sep 24, 2018
3e96b96
AF-1917: address review comments
sebastianmalysa-wf Sep 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions migrater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def get_factory_name(line):
>>> get_factory_name('UiFactory<DemoProps> Demo;')
'Demo'
"""
match = re.search(r' (\w+);', line)
match = re.search(r'(\w+);', line)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior to this boilerplate change, factory definitions would always be on one line. But with this change, dartfmt may break the right-hand-side of the factory assignment onto its own line like so:

UiFactory<NotificationManagerPrimitiveProps> NotificationManagerPrimitive =
    $NotificationManagerPrimitive;

We'll need to account for this; currently the script throws when it encounters this.

I think we should add a new method that determines whether or not the current line needs a factory update (which also accounts for lines that have already been transformed). Something like this:

diff --git a/migrater.py b/migrater.py
index c850d6d..4d25e7c 100644
--- a/migrater.py
+++ b/migrater.py
@@ -9,6 +9,7 @@ IGNORE_GENERATED_URI_COMMENT_LINE = '// ignore: uri_does_not_exist\n'
 GENERATED_PART_EXTENSION = '.generated.dart'
 DOLLAR_PROPS_USAGE_REGEX = r'(?:const|new)\s+\$Props\s*\(\s*([$A-Za-z0-9_.]+)\s*\)'
 DOLLAR_PROP_KEYS_USAGE_REGEX = r'(?:const|new)\s+\$PropKeys\s*\(\s*([$A-Za-z0-9_.]+)\s*\)'
+FACTORY_REGEX = r'^UiFactory(<\w+>) (\w+);'
 
 
 def get_class_name(line):
@@ -32,10 +33,10 @@ def get_factory_name(line):
     >>> get_factory_name('UiFactory<DemoProps> Demo;')
     'Demo'
     """
-    match = re.search(r'(\w+);', line)
+    match = re.search(FACTORY_REGEX, line)
     if not match:
         raise Exception('Could not parse factory name from:\n%s' % line)
-    return match.group(1)
+    return match.group(2)
 
 
 def get_part_name(path):
@@ -88,6 +89,10 @@ def has_dollar_prop_keys_usages(lines):
     return re.search(DOLLAR_PROP_KEYS_USAGE_REGEX, s, flags=re.MULTILINE) is not None
 
 
+def needs_factory_update(line):
+    return re.search(FACTORY_REGEX, line) is not None
+
+
 def update_dollar_props_usages(lines):
     s = ''.join(lines)
     match = re.search(DOLLAR_PROPS_USAGE_REGEX, s, flags=re.MULTILINE)
@@ -139,16 +144,12 @@ def factories_suggest(lines, path):
     need_part = False
 
     for line_number, line in enumerate(lines):
-        if line.startswith('UiFactory'):
+        if needs_factory_update(line):
             factory_name = get_factory_name(line)
-
-            if '$' + factory_name in line:
-                continue 
-            
             need_part = True
 
             ignore_line = '// ignore: undefined_identifier\n'
-            new_line = line.replace(';\n', ' = $%s;\n' % (factory_name))
+            new_line = line.replace(';\n', ' = $%s;\n' % factory_name)
 
             patches.append(codemod.Patch(line_number, new_lines=[
                 ignore_line,

if not match:
raise Exception('Could not parse factory name from:\n%s' % line)
return match.group(1)
Expand Down Expand Up @@ -140,9 +140,13 @@ def factories_suggest(lines, path):

for line_number, line in enumerate(lines):
if line.startswith('UiFactory'):
factory_name = get_factory_name(line)

if '$' + factory_name in line:
continue

need_part = True

factory_name = get_factory_name(line)
ignore_line = '// ignore: undefined_identifier\n'
new_line = line.replace(';\n', ' = $%s;\n' % (factory_name))

Expand Down Expand Up @@ -219,6 +223,10 @@ def props_metas_suggest(lines, path):
found_class_opening = False
class_body_is_empty = False
props_class_name = None
ignore_line = ' // ignore: undefined_identifier, undefined_class, const_initialized_with_non_constant_value\n'

if ignore_line in lines:
continue

for o, line_b in enumerate(lines[line_number:]):
if line_b.startswith('class ') or line_b.startswith('abstract class '):
Expand All @@ -240,9 +248,8 @@ def props_metas_suggest(lines, path):

last_class_def_line = lines[line_number + offset]
if class_body_is_empty:
last_class_def_line = last_class_def_line.replace('{}\n', '{\n')
last_class_def_line = last_class_def_line.replace('{}\n', '{\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one too many indents here


ignore_line = ' // ignore: undefined_identifier, undefined_class, const_initialized_with_non_constant_value\n'
meta_line = ' static const PropsMeta meta = $metaFor%s;\n' % props_class_name
# debug_line = 'line endings: %s' % line_endings

Expand Down
93 changes: 71 additions & 22 deletions test/migrater_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,68 +11,112 @@ void main() {
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/component_in_parts/component_in_part.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/component_in_parts/temp/component_in_part.dart');

makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('correctly converts components in a library', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/component_in_library');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/component_in_library/component_in_library.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/component_in_library/temp/component_in_library.dart');

makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('correctly converts components without props', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/component_without_props');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/component_without_props/component_without_props.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/component_without_props/temp/component_without_props.dart');

makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('correctly converts single \$PropKeys reference', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/mock_test_single_prop');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/mock_test_single_prop/mock_test_single_prop.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/mock_test_single_prop/temp/mock_test_single_prop.dart');

await makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
await setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('correctly coverts multiple \$PropKeys references', () async {
test('correctly converts multiple \$PropKeys references', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/mock_test_multiple_props');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/mock_test_multiple_props/mock_test_multiple_props.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/mock_test_multiple_props/temp/mock_test_multiple_props.dart');

await makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
await setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('correctly coverts single \$Prop reference', () async {
test('correctly converts single \$Prop reference', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/component_with_single_consumed_prop');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/component_with_single_consumed_prop/component_with_single_consumed_prop.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/component_with_single_consumed_prop/temp/component_with_single_consumed_prop.dart');

await makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
await setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('correctly coverts multiple \$Prop references', () async {
test('correctly converts multiple \$Prop references', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/component_with_multiple_consumed_props');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/component_with_multiple_consumed_props/component_with_multiple_consumed_props.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/component_with_multiple_consumed_props/temp/component_with_multiple_consumed_props.dart');

await makeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
await setupAndMakeComparison(pathToTestFixtureDirectory, pathToExpectedResults, pathToTestFile);
});

test('script is idempotent when modifiying a component', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/component_with_multiple_consumed_props');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/component_with_multiple_consumed_props/component_with_multiple_consumed_props.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/component_with_multiple_consumed_props/temp/component_with_multiple_consumed_props.dart');
final pathToTempDirectory = p.absolute('${pathToTestFixtureDirectory}/temp');
final pathToMigrater = p.absolute('migrater.py');

await setupTempDirectory(pathToTempDirectory, pathToTestFixtureDirectory);

/// Codemod the temporary directory contents twice.
await codemodTempDirectory(pathToMigrater, pathToTempDirectory);
await codemodTempDirectory(pathToMigrater, pathToTempDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of extra setup for each of these idempotent tests. Instead, I think we could accomplish this via an optional boolean param on the setupAndMakeComparison function like bool assertIdempotence. If set, then the function can run the script once, capture the result, run it again, capture the second result, and ensure that they're identical.


/// Compare expected results against codemod file in temp directory.
String expectedResults = new File(pathToExpectedResults).readAsStringSync();
String testFile = new File(pathToTestFile).readAsStringSync();

expect(testFile, equals(expectedResults));

await removeTempDirectory(pathToTestFixtureDirectory);
});

test('script is idempotent when modifiying a test', () async {
final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/mock_test_multiple_props');
final pathToExpectedResults = p.absolute('test/test_fixtures/after_codemod/mock_test_multiple_props/mock_test_multiple_props.dart');
final pathToTestFile = p.absolute('test/test_fixtures/before_codemod/mock_test_multiple_props/temp/mock_test_multiple_props.dart');
final pathToTempDirectory = p.absolute('${pathToTestFixtureDirectory}/temp');
final pathToMigrater = p.absolute('migrater.py');

await setupTempDirectory(pathToTempDirectory, pathToTestFixtureDirectory);

/// Code mode the temporary directory contents twice.
await codemodTempDirectory(pathToMigrater, pathToTempDirectory);
await codemodTempDirectory(pathToMigrater, pathToTempDirectory);

/// Compare expected results against codemod file in temp directory.
String expectedResults = new File(pathToExpectedResults).readAsStringSync();
String testFile = new File(pathToTestFile).readAsStringSync();

expect(testFile, equals(expectedResults));

await removeTempDirectory(pathToTestFixtureDirectory);
});
});
}

/// Creates a temp directory and copies the un-modified test fixtures to it. Then
/// migrater.py is run on these test fixtures to make the codemod changes.
Future<Null> setupAndCodemod(String pathToTestFixtureDirectory) async {
final pathToMigrater = p.absolute('migrater.py');
final pathToTempDirectory = p.absolute('${pathToTestFixtureDirectory}/temp');

/// Create a temporary directory and copy files from the specified before_codmod
/// test fixture directory.
Future<Null> setupTempDirectory(String pathToTempDirectory, String pathToTestFixtureDirectory) async {
await Process.run('/bin/bash',
['-c', 'mkdir -p temp && cp -r *.dart /$pathToTempDirectory'], workingDirectory: pathToTestFixtureDirectory);
}

/// Runs the migrater.py script to codemod files in the temporary directory.
Future<Null> codemodTempDirectory(String pathToMigrater, String pathToTempDirectory) async {
await Process.start('/bin/bash', ['-c', 'python $pathToMigrater'], workingDirectory: pathToTempDirectory).then((Process process) async {
await process.stdin.write('A\nA\n');
await process.stdin.close();
Expand All @@ -81,20 +125,25 @@ Future<Null> setupAndCodemod(String pathToTestFixtureDirectory) async {
}

/// Deletes the temp directory.
Future<Null> removeTempDir(String pathToTestFixtureDirectory) async {
Future<Null> removeTempDirectory(String pathToTestFixtureDirectory) async {
await Process.run('/bin/bash', ['-c', 'rm -R temp'], workingDirectory: pathToTestFixtureDirectory);
}

/// Makes the comparison between the expected result (migrated file) and the test file
/// (the file that is migrated during the test run in a temp directory).
Future<Null> makeComparison(String pathToTestFixtureDirectory, String pathToExpectedResults, String pathToTestFile) async {
await setupAndCodemod(pathToTestFixtureDirectory);
/// Setup and codemods the temporary directory and then makes the comparison
/// between the expected result (migrated file) and the test file
/// (the file that is migrated during setup).
Future<Null> setupAndMakeComparison(String pathToTestFixtureDirectory, String pathToExpectedResults, String pathToTestFile) async {
final pathToMigrater = p.absolute('migrater.py');
final pathToTempDirectory = p.absolute('${pathToTestFixtureDirectory}/temp');

await setupTempDirectory(pathToTempDirectory, pathToTestFixtureDirectory);
await codemodTempDirectory(pathToMigrater, pathToTempDirectory);

/// Compare expected results against codemod file in temp directory.
String expectedResults = new File(pathToExpectedResults).readAsStringSync();
String testFile = new File(pathToTestFile).readAsStringSync();

expect(testFile, equals(expectedResults));

await removeTempDir(pathToTestFixtureDirectory);
await removeTempDirectory(pathToTestFixtureDirectory);
}