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

Update Rely Setup #22

Merged

Conversation

bandersongit
Copy link
Contributor

@bandersongit bandersongit commented Apr 29, 2019

This separates the current Test.re file into three separate files and the current test project into a library and an executable so that the linkall flag can be used to improve developer experience when it comes to associating tests with the framework. Additionally I changed Pesy to no longer use the Test stanza/dune runtest due to the output swallowing behavior.

It looks like .re files written from the templates have a leading blank newline, however this was not introduced by my change (I verified that this happened with current master as well).

I tested this by running:

npm pack
tar -zxvf pesy-whatever
cd package
esy install
esy x (which pesy)

and then using the location from the esy x command to install a new project.

Currently when running this against an existing project it looks like that TestFramework.re and testExe/RunTests.re are created, but nothing gets overwritten. I'm not sure how we want to handle this incompatibility (FWIW existing tests still run, there are just new files that do nothing). I think figuring out what we want to do here is the only open question.

@ManasJayanth
Copy link
Member

ManasJayanth commented Apr 30, 2019

@bandersongit

This separates the current Test.re file into three separate files and the current test project into a library and an executable so that the linkall flag can be used to improve developer experience when it comes to associating tests with the framework. Additionally I changed Pesy to no longer use the Test stanza/dune runtest due to the output swallowing behavior.

This is great. I'll try it out soon as you mark the PR ready.

It looks like .re files written from the templates have a leading blank newline, however this was not introduced by my change (I verified that this happened with current master as well).

Yes, I have noticed this too. It's not your fault.

I tested this by running:
npm pack
untar
esy install
esy x (which pesy)
and then using the location from the esy x command to install a new project.

I'm not sure I follow this. You don't need the store path of the pesy binary to install a project (I'm assuming here by install you mean bootstrap). Projects are bootstrapped using globally installed pesy binary.

Currently when running this against an existing project it looks like that TestFramework.re and testExe/RunTests.re are created, but nothing gets overwritten.

This indeed in an interesting question. pesy doesn't have way to reconcile code. It merely creates new files and avoid overwriting if files already exist. I'd love to hear if you have any ideas on this.

@bandersongit
Copy link
Contributor Author

bandersongit commented Apr 30, 2019

@prometheansacrifice

I tested this by running:
npm pack
untar
esy install
esy x (which pesy)
and then using the location from the esy x command to install a new project.

I'm not sure I follow this. You don't need the store path of the pesy binary to install a project (I'm assuming here by install you mean bootstrap). Projects are bootstrapped using globally installed pesy binary.

Yeah I meant bootstrap, I updated to mention exactly what I did (cding into the untarred package and then running esy there). I didn't use the utilitities in e2e tests because I observed that they were in fact using the version I had installed globally and I didn't want to change that to be my local build, but I did want to test out my local build.

Currently when running this against an existing project it looks like that TestFramework.re and testExe/RunTests.re are created, but nothing gets overwritten.

This indeed in an interesting question. pesy doesn't have way to reconcile code. It merely creates new files and avoid overwriting if files already exist. I'd love to hear if you have any ideas on this.

I think the first step would be to separate initial project bootstrapping from the file generation based on a package.json. From there I could see doing something like:

a) if there is a package.json only run the code that generates dune files/whatever else from the package.json
b) Some scheme where we mark the version of pesy used to do initial bootstrapping (probably in some field in package.json) and then do something based on that

I think that a) makes sense if we can pretty confidently assert that everything we do given a package.json will be backwards compatible

All of that said, I am not particularly familiar with how Pesy works under the hood, so please consider my thoughts accordingly.

This is great. I'll try it out soon as you mark the PR ready.

All that remains (I believe) is resolving what we want to do with the reconciliation issue, I can remove the [WIP] tag if you like

@ManasJayanth
Copy link
Member

ManasJayanth commented Apr 30, 2019

@bandersongit Reconciliation of code needs some thought and discussion. We can continue doing so here. But as far as I PR is concerned, lets scope it within the Rely setup.

@bandersongit bandersongit changed the title [WIP] Update Rely Setup Update Rely Setup Apr 30, 2019
@bandersongit
Copy link
Contributor Author

@bandersongit Reconciliation of code needs some thought and discussion. We can continue doing so here. But as far as I PR is concerned, lets scope it within the Rely setup.

Okay then I think this is good to go purely in terms of setting up Rely, and it just has the known issue of adding new files for existing pesy projects.

@ManasJayanth
Copy link
Member

@bandersongit

Thank you for this PR. I'm bringing it in. I have a few questions about the code reconciliation.

Currently when running this against an existing project it looks like that TestFramework.re and testExe/RunTests.re are created, but nothing gets overwritten.

This indeed in an interesting question. pesy doesn't have way to reconcile code. It merely creates new files and avoid overwriting if files already exist. I'd love to hear if you have any ideas on this.

I think the first step would be to separate initial project bootstrapping from the file generation based on a package.json. From there I could see doing something like:

a) if there is a package.json only run the code that generates dune files/whatever else from the package.json
b) Some scheme where we mark the version of pesy used to do initial bootstrapping (probably in some field in package.json) and then do something based on that.

Just making sure I understand you correctly: you noticed TestFramework.re and testExe/RunTests.re are created when you start afresh, but existing project don't seem to receive these updates (i.e. running them in existing projects that don't have the new Rely setup doesn't upgrade the Rely setup). This is what made me think in terms of reconciliation: bringing Rely setup to a new state based on an update operation by pesy.

Feel free to correct me if I misunderstood, it seems that you are suggesting we figure a way to distinguish between pesy that bootstraps and pesy that updates config. Actually, pesy does work in two modes - as bootstrapper and as config reconciler. It does have a way to function as one at a time. What it isnt able to do is correctly bring setup related code to a new state (ie the new set up in the event that tool receives updates). Which is why it leaves the code alone.

We have come across this wrt CI setup as well. Let me know your thoughts. We can keep this thread alive. Once we have some clarity, we can write up an issue for others to see and contribute 🙂 Thanks for bring this up btw 👍

@ManasJayanth ManasJayanth merged commit 661037d into esy:master May 1, 2019
@bandersongit
Copy link
Contributor Author

Just making sure I understand you correctly: you noticed TestFramework.re and testExe/RunTests.re are created when you start afresh, but existing project don't seem to receive these updates (i.e. running them in existing projects that don't have the new Rely setup doesn't upgrade the Rely setup). This is what made me think in terms of reconciliation: bringing Rely setup to a new state based on an update operation by pesy.

To clarify, when I ran against an existing pesy project (created with the master version of pesy), it did get the new files (however old files weren't overwritten), glad to hear that theoretically it should work in the way I described however! I'll see if I can repro again and that I wasn't doing anything weird

I can definitely see this being a problem with ci as well, however I don't have thoughts on how to address that. I think that we can also upgrade the rely integration with ci by default though (run with the junit reporter, configure azure to look for the output of that, and run Rely in ciMode to prevent test/describeOnly from running in CI).

@ManasJayanth
Copy link
Member

To clarify, when I ran against an existing pesy project (created with the master version of pesy), it did get the new files (however old files weren't overwritten), glad to hear that theoretically it should work in the way I described however

No, it won't work as you described i.e. overwrite/delete old files and place the new setup files. This is missing from pesy. We don't know what the right behaviour is!

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.

3 participants