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

support for test code reuse #320

Merged
merged 17 commits into from
May 27, 2020
Merged

support for test code reuse #320

merged 17 commits into from
May 27, 2020

Conversation

lsylvestre
Copy link
Contributor

about #169 and #174.

This PR offers a support for test code reuse.

It allows the programmer to add an optional file called depend.txt (in the exercises directory) which declares a list of dependencies as follows :

../lib/m1.ml   # just a comment
../lib/m1.mli

../lib/m2.ml

Thus, these ml and mli files are copied, ciphered and will be loaded in the toplevel during the grading, just before test.ml. It is therefore possible to use the modules M1 and M2 in test.ml and, furthermore, to use M1 in M2.

@yurug
Copy link
Collaborator

yurug commented Feb 8, 2020

Thank you for this proposal. Can you document this feature in a new file located in docs, please?

@yurug
Copy link
Collaborator

yurug commented Feb 19, 2020

Ping @lsylvestre

@lsylvestre
Copy link
Contributor Author

Hi @yurug,
I am sorry for the long delay...
Based on your comments and suggestions, I have improved my solution and written a new step in docs/tutorials. Thank you for your review.

To prevent a warning, I would like to add the following fragment in translations/fr.po:

#: src/grader/grading.ml:173,43--80
msgid "while loading user dependencies"
msgstr "lors du chargement des dépendances"

but it seems to produce a conflict.

@yurug
Copy link
Collaborator

yurug commented Apr 22, 2020

Can you give more details about your conflict? What kind of conflicts is it?


Each declaration in **depend.txt** is a single line containing the relative path of an *.ml* or *.mli* file. The order of the *.ml* declarations specifies the order in which each module is loaded in the grading environment.

By default each dependency *foo.ml* is isolated in a module *Foo*, which can be constrained by the content of an optional signature file *foo.mli*. Furthermore, we can add an annotation `@included` at the beginning of a file *foo.ml* to denote that all the bindings of *foo.ml* is are evaluated in the toplevel environment (and not in a module *Foo*).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use shorter lines in text files (80 columns max.).

Here, who is "we"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
By default each dependency *foo.ml* is isolated in a module *Foo*, which can be constrained by the content of an optional signature file *foo.mli*. Furthermore, we can add an annotation `@included` at the beginning of a file *foo.ml* to denote that all the bindings of *foo.ml* is are evaluated in the toplevel environment (and not in a module *Foo*).
By default each dependency *foo.ml* is isolated in a module *Foo*, which can be constrained by the content of an optional signature file *foo.mli*. Furthermore, we can add an annotation `@included` at the beginning of a file *foo.ml* to denote that all the bindings of *foo.ml* are evaluated in the toplevel environment (and not in a module *Foo*).

Copy link
Collaborator

Choose a reason for hiding this comment

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

You should provide an example, especially to show what is exactly the syntax for annotations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you.

  • ok for 80 columns max.
  • "we" is "the authors of the exercise". I rephrase this in "an annotation [@@@included] can be used...".
  • below, there is a complete example using this annotation.
    Would you like me to add another simpler example ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No that's fine.

Co-Authored-By: Yann Régis Gianas <yrg@pps.univ-paris-diderot.fr>
Copy link
Collaborator

@yurug yurug left a comment

Choose a reason for hiding this comment

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

Commit 36f6272 should probably have been in another PR but let's forget about this.

Remember that **Test_lib** internally requires a user-defined sampler `sample_peano : unit -> peano` to generate value of type `peano`. This sampler has to be present in the toplevel environment -- and not in a module -- in order to be found by the introspection primitives during grading. Therefore, we define this sampler in a file starting by the annotation `@included`.
- **samples.ml**:
```ocaml
@included
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the standard syntax:

[@@@included]

would have the benefits of making this .ml file accepted by the compiler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have switched to the standard syntax.

Note that the parsing of the annotation is hard-coded in grading.ml as a string comparison:

match String.trim (String.sub content 0 i) with 
| "[@@@included]" -> ...

Is this the right way to do it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's probably fine.

A cleaner way would be to inspect the module syntax tree but I am not sure if that is easy for you to get this tree at this point of the process.

@lsylvestre
Copy link
Contributor Author

lsylvestre commented Apr 22, 2020

Can you give more details about your conflict? What kind of conflicts is it?

The conflict was due to my outdated version of the file translations/fr.po. It is a ~"simple merge conflicts that can be resolve on GitHub using the conflict editor".
Do you want me to commit my change in translations/fr.po again ?

@lsylvestre
Copy link
Contributor Author

lsylvestre commented Apr 22, 2020

Commit 36f6272 should probably have been in another PR but let's forget about this.

Apparently, these links are dead in the actual version of learn-ocaml.

lsylvestre and others added 6 commits April 22, 2020 14:31
Co-Authored-By: Yann Régis Gianas <yrg@pps.univ-paris-diderot.fr>
Co-Authored-By: Yann Régis Gianas <yrg@pps.univ-paris-diderot.fr>
Co-Authored-By: Yann Régis Gianas <yrg@pps.univ-paris-diderot.fr>
@yurug
Copy link
Collaborator

yurug commented Apr 23, 2020

Can you give more details about your conflict? What kind of conflicts is it?

The conflict was due to my outdated version of the file translations/fr.po. It is a ~"simple merge conflicts that can be resolve on GitHub using the conflict editor".
Do you want me to commit my change in translations/fr.po again ?

Yes please.

@lsylvestre
Copy link
Contributor Author

Can you give more details about your conflict? What kind of conflicts is it?

The conflict was due to my outdated version of the file translations/fr.po. It is a ~"simple merge conflicts that can be resolve on GitHub using the conflict editor".
Do you want me to commit my change in translations/fr.po again ?

Yes please.

It is done. There is a conflict as expected.

@yurug
Copy link
Collaborator

yurug commented May 27, 2020

Thanks! We are done here, @lsylvestre, great work!

@yurug yurug merged commit 42d8127 into ocaml-sf:master May 27, 2020
@lsylvestre
Copy link
Contributor Author

Thank you @yurug, it was my pleasure.

erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request Jun 18, 2021
This ensures learn-ocaml-client.master is still able to query a server
version learn-ocaml.0.12.

Minimal working example:
  cd .../learn-ocaml
  docker run --name=locaml -d -it -p 8080:8080 -v "$PWD/demo-repository:/repository" ocamlsf/learn-ocaml:0.12
  TOKEN=$(docker logs locaml | grep -e 'Initial teacher token created:' | sed -e 's/^.*: //')
  # then
  git checkout 42d8127
  opam install . --deps-only --locked -y
  opam install -y opam-installer.$(opam config var opam-version)
  eval $(opam env)
  make
  make opaminstall
  learn-ocaml-client init -s http://localhost:8080 -t "$TOKEN"
  touch demo.ml
  learn-ocaml-client grade demo.ml

Output before the fix:
  Fatal error: exception Json_encoding.Cannot_destruct(_)
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request Jun 18, 2021
This ensures learn-ocaml-client.master is still able to query a server
version learn-ocaml.0.12.

Minimal working example:
  cd .../learn-ocaml
  docker run --name=locaml -d -it -p 8080:8080 -v "$PWD/demo-repository:/repository" ocamlsf/learn-ocaml:0.12
  TOKEN=$(docker logs locaml | grep -e 'Initial teacher token created:' | sed -e 's/^.*: //')
  # then
  git checkout 42d8127
  opam install . --deps-only --locked -y
  opam install -y opam-installer.$(opam config var opam-version)
  eval $(opam env)
  make
  make opaminstall
  learn-ocaml-client init -s http://localhost:8080 -t "$TOKEN"
  touch demo.ml
  learn-ocaml-client grade demo.ml

Output before the fix:
  Fatal error: exception Json_encoding.Cannot_destruct(_)
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
yurug pushed a commit that referenced this pull request Jun 24, 2021
This ensures learn-ocaml-client.master is still able to query a server
version learn-ocaml.0.12.

Minimal working example:
  cd .../learn-ocaml
  docker run --name=locaml -d -it -p 8080:8080 -v "$PWD/demo-repository:/repository" ocamlsf/learn-ocaml:0.12
  TOKEN=$(docker logs locaml | grep -e 'Initial teacher token created:' | sed -e 's/^.*: //')
  # then
  git checkout 42d8127
  opam install . --deps-only --locked -y
  opam install -y opam-installer.$(opam config var opam-version)
  eval $(opam env)
  make
  make opaminstall
  learn-ocaml-client init -s http://localhost:8080 -t "$TOKEN"
  touch demo.ml
  learn-ocaml-client grade demo.ml

Output before the fix:
  Fatal error: exception Json_encoding.Cannot_destruct(_)
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
  Called from unknown location
@erikmd erikmd mentioned this pull request Oct 5, 2021
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