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.

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

Add e2e test for Purescript #615

Merged
merged 4 commits into from
Sep 5, 2024
Merged

Add e2e test for Purescript #615

merged 4 commits into from
Sep 5, 2024

Conversation

kharus
Copy link
Collaborator

@kharus kharus commented Aug 30, 2024

@kosmikus , @fendor
As I mentioned I'm trying to set up minimal E2E testing for PS Transpiler before trying to refactor it.

As a first step I copied-and-pasted code from main just to run the transpiler without using any driver infra. Test code is very messy and can't even be used to write another test without complete copy and paste.

I was thinking maybe use driver infra but in this case should it be pulled out to other module where I can import it from?

Can you please provide your input on

  • Potential usage on driver infra in e2e tests
  • Further ideas on improving e2e tests itself.

I can set up a short call if it's faster to discuss over zoom.

@kharus kharus requested review from kosmikus and fendor August 30, 2024 02:50
Copy link
Contributor

@fendor fendor left a comment

Choose a reason for hiding this comment

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

The structure looks good, I have some comments regarding handling errors.

Further, I think it would be nice if we refactor this code into reusable functions.
E.g., I am thinking of a function setupNlgEnv which has the type Interpreted -> IO NLGEnv, or something like that

lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
lib/haskell/natural4/test/LS/XPile/PurescriptSpec.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@fendor fendor left a comment

Choose a reason for hiding this comment

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

LGTM, two small changes :)

Comment on lines 104 to 105
must_sing_purs <- runIO $ transpileFile "must_sing"
it "convert must sing to Purescript" $ goldenGeneric "must_sing" must_sing_purs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
must_sing_purs <- runIO $ transpileFile "must_sing"
it "convert must sing to Purescript" $ goldenGeneric "must_sing" must_sing_purs
it "convert must sing to Purescript" $ do
must_sing_purs <- transpileFile "must_sing"
goldenGeneric "must_sing" must_sing_purs

Then it is a perfect test case, imo!

Comment on lines 50 to 51
putStrLn "natural4: encountered error while obtaining allNLGEnv"
DF.traverse_ putStrLn allNLGEnvErrors
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
putStrLn "natural4: encountered error while obtaining allNLGEnv"
DF.traverse_ putStrLn allNLGEnvErrors
error $ unlines $ "natural4: encountered error while obtaining allNLGEnv" : allNLGEnvErrors

@kharus kharus marked this pull request as ready for review September 5, 2024 01:37
@kharus kharus merged commit 78a087e into main Sep 5, 2024
3 checks passed
@kharus kharus deleted the add-purescript-e2e branch September 5, 2024 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants