Introduce plugin for migrating scalatest #572

ketkarameya · 2023-08-11T23:04:00Z

Adding plugin to migrate scalatest

lazaroclapp

Do we want to call this scala_test? Looking at the directory path, I'd expect that to be some testing for Piranha, not a tool to migrate tests. Maybe scala_test_migrator?

lazaroclapp · 2023-08-14T19:11:10Z

plugins/pyproject.toml

+
+[tool.poetry.dependencies]
+python = "^3.9"
+polyglot_piranha = "*"


What does "*" mean here? Any version? Latest?

yes * means latest.

Do we want to be silently latest? Could we have this just be in sync with the current released Piranha version? (Also, pytest below should probably be set to a concrete library and we should manually keep the dep up to date, no?). Basically, just in terms of reproducibility I am wary of dependencies without a explicit version.

plugins/pyproject.toml

lazaroclapp · 2023-08-14T19:12:01Z

plugins/scala_test/README.md

@@ -0,0 +1,26 @@
+# `scalatest` Migration Plugin 
+


This needs a description/explanation of what this is, before the Usage instructions.

Would also be a good point to note if this is a WIP or already functional and for which cases.

lazaroclapp · 2023-08-14T19:13:33Z

plugins/scala_test/main.py

+from update_imports import update_imports
+
+def _parse_args():
+    parser = argparse.ArgumentParser(description="Migrates scala tests!!!")


Migrates them to what or from what? Also, longer term, do we need a parameter for a target version which affects which mappings we use?

I have updated the app description, to reflect the version number that we want to update to.
Added a cli-arg with default value .

lazaroclapp · 2023-08-14T19:19:20Z

plugins/scala_test/recipes.py

+
+
+
+def replace_import_rules_edges(


Suggested change

def replace_import_rules_edges(

def replace_import_rules_and_edges(

Otherwise it reads as generating the "rules' edges" and it might be surprising that it also returns a list of rules.

Btw, these methods and update_imports could use docs. Maybe even those in test_update_imports too, but I leave that up to you.

done! Credits - CoPilot

lazaroclapp · 2023-08-14T19:23:51Z

plugins/scala_test/recipes.py

+        query="((identifier) @x (#eq? @x \"@search_heuristic\"))",
+        holes={"search_heuristic"},
+    )
+    e1 = OutgoingEdges("find_relevant_files", to=[f"update_import"], scope="File")


Interesting. This runs every rule in the "update_import" group if "find_relevant_files" matches, right?

exactly. The search heuristic narrows down the scope and prevents us from parsing the entire code base. And then we apply these "update import" rules only within these files

lazaroclapp · 2023-08-14T19:26:05Z

plugins/scala_test/tests/resources/input/sample.scala

+import org.apache.spark.sql.Row
+import org.apache.spark.sql.types.{DoubleType, StringType, StructField, StructType}
+import org.scalatest.{BeforeAndAfter, Matchers}
+import org.scalatest.mock.MockitoSugar


Shouldn't we have a case where an import like import pkg1.pkg2.{A, B} is replaced as import pkg1.pkg2.{C, B} and also the simple import pkg3.pkg4.D -> import pkg5.pkg6.E case?

hmmm. Actually the solution u suggest looks clean when the before and after type have a significant overlap in their qualified name. Else we have to "infer" the level to split the type name.

From: a.b.c.D to a.e.f.g.H.
Before: import a.b.c.{D, E}
After: import a.{b.c.E, e.f.H}
or add some extra logic to heuristically decide when it is not a good idea to add a nested import, and add a simple import at those times.

From: a.b.c.D to a.b.c.H.
Before: import a.b.c.{D, E}
After: import a.b.c.{H, E}

( actually @raviagarwal7 suggested adding simple import and keep the rewrite logic less complicated. We believe that we can use Scala linters to re-org the imports like we want to)

Not sure I understand this. I am not saying the rewrite done in these tests is incorrect, but I think there are missing cases given the rewrite rules you added. Specifically the update_simple_import_{...} rules. My question here is about test coverage for the rules/logic added.

lazaroclapp · 2023-08-14T19:28:41Z

plugins/scala_test/tests/test_update_imports.py

+    summary = update_imports("plugins/scala_test/tests/resources/input/", dry_run=True)
+    assert is_as_expected("plugins/scala_test/tests/resources/", summary)
+
+def is_as_expected(path_to_scenario, output_summary):


Wonder if there is a clean way to avoid the duplication between this code and the top level test harness logic. Maybe a shared test utilities library? Not a big deal, but if every plugin will have it's own copy of this code that might be a pain when you need to update something.

I agree. I wanted to that . I will eventually extract out a commons .
This could moved to commons/test_utilities
I believe that replace_imports could also be a part of commons/scala

ketkarameya

Addressed comments - 43a7628

ketkarameya · 2023-08-15T00:52:50Z

plugins/pyproject.toml

+
+[tool.poetry.dependencies]
+python = "^3.9"
+polyglot_piranha = "*"


yes * means latest.

plugins/pyproject.toml

ketkarameya · 2023-08-15T01:12:43Z

plugins/scala_test/README.md

@@ -0,0 +1,26 @@
+# `scalatest` Migration Plugin 
+


ketkarameya · 2023-08-15T01:20:35Z

plugins/scala_test/main.py

+from update_imports import update_imports
+
+def _parse_args():
+    parser = argparse.ArgumentParser(description="Migrates scala tests!!!")


I have updated the app description, to reflect the version number that we want to update to.
Added a cli-arg with default value .

ketkarameya · 2023-08-15T01:29:28Z

plugins/scala_test/recipes.py

+        query="((identifier) @x (#eq? @x \"@search_heuristic\"))",
+        holes={"search_heuristic"},
+    )
+    e1 = OutgoingEdges("find_relevant_files", to=[f"update_import"], scope="File")


exactly. The search heuristic narrows down the scope and prevents us from parsing the entire code base. And then we apply these "update import" rules only within these files

ketkarameya · 2023-08-15T16:33:31Z

plugins/scala_test/recipes.py

+
+
+
+def replace_import_rules_edges(


done! Credits - CoPilot

ketkarameya · 2023-08-15T16:42:01Z

plugins/scala_test/tests/resources/input/sample.scala

+import org.apache.spark.sql.Row
+import org.apache.spark.sql.types.{DoubleType, StringType, StructField, StructType}
+import org.scalatest.{BeforeAndAfter, Matchers}
+import org.scalatest.mock.MockitoSugar


hmmm. Actually the solution u suggest looks clean when the before and after type have a significant overlap in their qualified name. Else we have to "infer" the level to split the type name.

From: a.b.c.D to a.e.f.g.H.
Before: import a.b.c.{D, E}
After: import a.{b.c.E, e.f.H}
or add some extra logic to heuristically decide when it is not a good idea to add a nested import, and add a simple import at those times.

From: a.b.c.D to a.b.c.H.
Before: import a.b.c.{D, E}
After: import a.b.c.{H, E}

( actually @raviagarwal7 suggested adding simple import and keep the rewrite logic less complicated. We believe that we can use Scala linters to re-org the imports like we want to)

ketkarameya · 2023-08-15T16:44:28Z

plugins/scala_test/tests/test_update_imports.py

+    summary = update_imports("plugins/scala_test/tests/resources/input/", dry_run=True)
+    assert is_as_expected("plugins/scala_test/tests/resources/", summary)
+
+def is_as_expected(path_to_scenario, output_summary):


I agree. I wanted to that . I will eventually extract out a commons .
This could moved to commons/test_utilities
I believe that replace_imports could also be a part of commons/scala

lazaroclapp · 2023-08-15T17:52:17Z

plugins/pyproject.toml

+
+[tool.poetry.dependencies]
+python = "^3.9"
+polyglot_piranha = "*"


Do we want to be silently latest? Could we have this just be in sync with the current released Piranha version? (Also, pytest below should probably be set to a concrete library and we should manually keep the dep up to date, no?). Basically, just in terms of reproducibility I am wary of dependencies without a explicit version.

lazaroclapp · 2023-08-15T17:55:22Z

plugins/scala_test/recipes.py

+    It supports both simple and nested imports. While the simple imports are replaced directly, the nested imports are deleted and the new type is imported (as a simple non-nested import).
+    Assume that the target type is "a.b.c.d" and the new type is "x.y.z". Then the following rules are generated:
+    import a.b.c.d -> import x.y.z
+    import a.b.c.{d, e} -> import x.y.z \n import a.b.c.{d}


Suggested change

import a.b.c.{d, e} -> import x.y.z \n import a.b.c.{d}

import a.b.c.{d, e} -> import x.y.z \n import a.b.c.{e}

I believe you meant e here, since d is the one getting replaced...

beyeu107 · 2023-08-15T21:37:43Z

Rules LGTM

ketkarameya requested a review from lazaroclapp August 11, 2023 23:04

ketkarameya force-pushed the scala-test-plugin branch from d4f3ce8 to bf9b4d3 Compare August 11, 2023 23:05

ketkarameya changed the base branch from master to scala-file-scope August 11, 2023 23:06

ketkarameya requested a review from raviagarwal7 August 14, 2023 01:42

ketkarameya added 2 commits August 13, 2023 18:46

Adding plugin to migrate scalatest

5b18385

Update the plugin

f075ebc

ketkarameya force-pushed the scala-test-plugin branch from 27bd33a to f075ebc Compare August 14, 2023 01:47

ketkarameya changed the base branch from scala-file-scope to master August 14, 2023 01:47

Update the plugin

5fbdb7b

lazaroclapp reviewed Aug 14, 2023

View reviewed changes

Update the plugin

43a7628

ketkarameya commented Aug 15, 2023

View reviewed changes

lazaroclapp reviewed Aug 15, 2023

View reviewed changes

ketkarameya force-pushed the master branch from d802f3e to 3955145 Compare September 7, 2023 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce plugin for migrating scalatest #572

Introduce plugin for migrating scalatest #572

ketkarameya commented Aug 11, 2023

lazaroclapp left a comment

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 15, 2023

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 15, 2023

lazaroclapp Aug 14, 2023

ketkarameya Aug 15, 2023

ketkarameya left a comment •

edited

Loading

ketkarameya Aug 15, 2023

ketkarameya Aug 15, 2023

ketkarameya Aug 15, 2023

ketkarameya Aug 15, 2023

ketkarameya Aug 15, 2023

ketkarameya Aug 15, 2023

ketkarameya Aug 15, 2023

lazaroclapp Aug 15, 2023

lazaroclapp Aug 15, 2023

beyeu107 commented Aug 15, 2023

	def replace_import_rules_edges(
	def replace_import_rules_and_edges(

	import a.b.c.{d, e} -> import x.y.z \n import a.b.c.{d}
	import a.b.c.{d, e} -> import x.y.z \n import a.b.c.{e}

Introduce plugin for migrating scalatest #572

Are you sure you want to change the base?

Introduce plugin for migrating scalatest #572

Conversation

ketkarameya commented Aug 11, 2023

lazaroclapp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

From: a.b.c.D to a.e.f.g.H. Before: import a.b.c.{D, E} After: import a.{b.c.E, e.f.H} or add some extra logic to heuristically decide when it is not a good idea to add a nested import, and add a simple import at those times.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketkarameya left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

From: a.b.c.D to a.e.f.g.H. Before: import a.b.c.{D, E} After: import a.{b.c.E, e.f.H} or add some extra logic to heuristically decide when it is not a good idea to add a nested import, and add a simple import at those times.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beyeu107 commented Aug 15, 2023

From: `a.b.c.D` to `a.e.f.g.H`.
Before: `import a.b.c.{D, E}`
After: `import a.{b.c.E, e.f.H}`
or add some extra logic to heuristically decide when it is not a good idea to add a nested import, and add a simple import at those times.

ketkarameya left a comment •

edited

Loading

From: `a.b.c.D` to `a.e.f.g.H`.
Before: `import a.b.c.{D, E}`
After: `import a.{b.c.E, e.f.H}`
or add some extra logic to heuristically decide when it is not a good idea to add a nested import, and add a simple import at those times.