AF-2152: add more tests to cover additions made in AF-1928 #4

sebastianmalysa-wf · 2018-08-23T21:04:22Z

DO NOT MERGE UNTIL #2 IS MERGED

Add more test to cover additions made in AF-1928.

@corwinsheahan-wf @evanweible-wf @maxwellpeterson-wf

…to AF-2152

rmconsole-wf · 2018-08-23T21:04:45Z

Merge Requirements Met ✅

Request Rosie to automerge this pull request by including @Workiva/release-management-p in a comment.

Required artifacts are reported in build. See documentation for more details on build artifact requirements.
- python requires one of: pip.lock, python3_pip_deps.txt, python2_pip_deps.txt

General Information

Ticket(s):

AF-2152 | remove
AF-1928 | remove

Code Review(s): #4

Reviewers: evanweible-wf

Additional Information

Watchlist Notifications: None

	When this pull is merged I will add it to the following release:
	Version: over_react_codemod 0.1.0
	Release Ticket(s): None

Note: This is a shortened report. Click here to view Rosie's full evaluation.
Last updated on Friday, September 21 12:39 AM CST

aviary-wf · 2018-08-23T21:04:49Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on HipChat: InfoSec Forum.

…F-2152

evanweible-wf · 2018-09-19T17:00:34Z

pubspec.yaml

@@ -0,0 +1,16 @@
+name: over_react_codemode


extra e at the end

evanweible-wf · 2018-09-19T17:00:59Z

Dockerfile

+WORKDIR /build/
+ADD . /build/
+ENV TERM=linux
+ENV TERMINFO=/etc/terminfo


I remember you explaining this to me before, but could you remind me why this is needed?

These are need to run 'migrater.py' within CI so the tests run properly.

evanweible-wf · 2018-09-19T17:01:55Z

Dockerfile

+ENV TERM=linux
+ENV TERMINFO=/etc/terminfo
+RUN echo "Install codemod" && \
+        pip install git+https://github.com/georgelesica-wf/codemod@dart-convert && \


George's PR to codemod was accepted, so I believe we can install the official facebook version of codemod now

We still need George's branch because they haven't released the changes he made.

evanweible-wf · 2018-09-19T17:24:34Z

test/test_fixtures/after_codemod/component_in_library/component_in_library.dart

+
+  // ignore: deprecated_member_use
+  @Required(message: 'This Prop is Required for testing purposes.')
+  var required;


The alternative for the deprecated @Required is @requiredProp – let's use that and then we won't also need to ignore the deprecated member use.

evanweible-wf · 2018-09-19T17:24:43Z

test/test_fixtures/after_codemod/component_in_library/component_in_library.dart

+
+  // ignore: deprecated_member_use
+  @Required(isNullable: true, message: 'This prop can be set to null!')
+  var nullable;


@nullableRequiredProp

evanweible-wf · 2018-09-19T18:21:31Z

test/migrater_test.dart

+/// 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');


Since this doesn't change, you could define it as a final var at the top of the file.

evanweible-wf · 2018-09-19T18:27:56Z

test/migrater_test.dart

+    test('correctly converts components with part directive', () async {
+      final pathToTestFixtureDirectory = p.absolute('test/test_fixtures/before_codemod/component_in_parts');
+      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');


Since the makeComparison method is creating the temp directory and running codemod on it, it should be the one to handle finding the result of the codemod script. In other words, let's update makeComparison to figure this path out on its own instead of needing to define it for each test and pass it in.

evanweible-wf · 2018-09-19T18:30:25Z

test/migrater_test.dart

+  await Process.run('/bin/bash',
+      ['-c', 'mkdir -p temp && cp -r *.dart /$pathToTempDirectory'], workingDirectory: pathToTestFixtureDirectory);
+
+  await Process.start('/bin/bash', ['-c', 'python $pathToMigrater'], workingDirectory: pathToTempDirectory).then((Process process) async {


Do we need to explicitly run the script via /bin/bash like this? Or would this work:

Process.start('python', [pathToMigrater], workingDirectory: pathToTempDirectory)...

evanweible-wf · 2018-09-19T18:31:19Z

test/migrater_test.dart

+      ['-c', 'mkdir -p temp && cp -r *.dart /$pathToTempDirectory'], workingDirectory: pathToTestFixtureDirectory);
+
+  await Process.start('/bin/bash', ['-c', 'python $pathToMigrater'], workingDirectory: pathToTempDirectory).then((Process process) async {
+    await process.stdin.write('A\nA\n');


We should probably add a flag to the migrater.py script that allows us to automatically apply all changes without requiring input from stdin, that way we can just include that flag in our command above

There is no way for codemod to run in a way that will accept all changes, at least not in the current release. A PR (facebookarchive/codemod#48) was made that adds the flag we want, but it hasn't been released :(

That should be in George's branch though, so I think I can get this to work.

evanweible-wf · 2018-09-19T18:34:12Z

test/migrater_test.dart

+
+/// 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 {


I think a slightly simpler/cleaner approach would be for this function to accept:

The before File, which it will copy to a temp directory and run the codemod script on

The after File, which it will compare against the result of the codemod script

…ckerfile

…o one more time

…d still

…file

evanweible-wf

Looks great!!

evanweible-wf · 2018-09-21T05:29:22Z

QA +1

Tests pass!

@Workiva/release-management-p

rmconsole-wf · 2018-09-21T05:29:38Z

@evanweible-wf I will not merge this because:

This PR will not Auto-Merge because a project is excluded from the
Automerge Projects on this repo.
Project: AF
If you are the maintainer of this repo and would like to allow this project to automerge,
please update the automerge projects list on the Rosie Control panel.
'Hold Auto-Merge' label is applied

rmconsole-wf · 2018-09-21T05:39:35Z

+1 from RM

sebastianmalysa-wf added 2 commits August 20, 2018 15:20

AF-1915: add test cases for new Factory boiler plate codemod changes

053e3a7

Merge branch 'props_meta' of github.com:Workiva/over_react_codemod in…

1829643

…to AF-2152

rmconsole-wf added Merge Requirements Unmet Hold Auto-Merge labels Aug 23, 2018

This was referenced Aug 23, 2018

AF-1916: add functional tests for new factory boiler plate Workiva/over_react#175

Closed

AF-1915: add test cases for new Factory boiler plate codemod changes #3

Closed

AF-1917: Make over_react_codemod script idempotent #5

Merged

Merge branch 'master' of github.com:Workiva/over_react_codemod into A…

a9ab06d

…F-2152

evanweible-wf requested changes Sep 19, 2018

View reviewed changes

sebastianmalysa-wf added 9 commits September 20, 2018 08:55

AF-2152: clean up tests and address review comments

35cd60a

AF-2152: address more comments

b700267

AF-2152: add ENV TERM=linux and ENV TERMINFO=/etc/terminfo back to Do…

c266120

…ckerfile

AF-2152: use George's branch

ae03264

AF-2152: trying removing ENV TERM=linux and ENV TERMINFO=/etc/terminf…

d305667

…o one more time

AF-2152: nope ENV TERM=linux and ENV TERMINFO=/etc/terminfo are neede…

4b7e8b4

…d still

AF-2152: set the temp directory path to a variable

db2480c

AF-2152: add one more item to the dollar prop key usage before/after …

7d81ebb

…file

AF-2152: add missing line

639dcd3

evanweible-wf approved these changes Sep 21, 2018

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Sep 21, 2018

rmconsole-wf added the RM Ready label Sep 21, 2018

evanweible-wf removed the Hold Auto-Merge label Sep 21, 2018

rmconsole5-wk merged commit 6ea4e10 into master Sep 21, 2018

rmconsole5-wk deleted the AF-2152 branch September 21, 2018 05:39

rmconsole-readonly-wk mentioned this pull request Jan 23, 2019

AF-3863 Release over_react_codemod 1.0.0 #15

Merged

AF-2152: add more tests to cover additions made in AF-1928 #4

AF-2152: add more tests to cover additions made in AF-1928 #4

Uh oh!

Conversation

sebastianmalysa-wf commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmconsole-wf commented Aug 23, 2018 • edited by rmconsole5-wk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Requirements Met ✅

General Information

Additional Information

Uh oh!

aviary-wf commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Insights

Action Items

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanweible-wf left a comment

Choose a reason for hiding this comment

Uh oh!

evanweible-wf commented Sep 21, 2018

Uh oh!

rmconsole-wf commented Sep 21, 2018

Uh oh!

rmconsole-wf commented Sep 21, 2018

Uh oh!

Uh oh!

sebastianmalysa-wf commented Aug 23, 2018 •

edited

Loading

rmconsole-wf commented Aug 23, 2018 •

edited by rmconsole5-wk

Loading

aviary-wf commented Aug 23, 2018 •

edited

Loading