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 file content #577

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

add file content #577

wants to merge 38 commits into from

Conversation

BrentBlanckaert
Copy link
Collaborator

No description provided.

@BrentBlanckaert BrentBlanckaert self-assigned this Jan 11, 2025
@BrentBlanckaert BrentBlanckaert added the enhancement New feature or request label Jan 11, 2025
@BrentBlanckaert
Copy link
Collaborator Author

BrentBlanckaert commented Feb 18, 2025

Documentation for usage of files

This documentation will discuss all the changes made regarding
the IO of a test.

Files

Files are used to describe files that can be added as input for a test and will provide the student with this file in Dodona. This is done in the following way:

files:
  - name: "animal.txt"
    url: "media/workdir/animal.txt"

files is not to be confused with file which will specify the contents and location of a file the code of a student should generate. This can be done in the following way:

file:
  content: "animal.txt" # is the content that the file should have
  location: "media/workdir/animal.txt" # is the location of the file
  oracle: ...

There are several issues with this:

  • The usage of the names files and file is very confusing
  • In content you had to use a file and can't just specify the content
  • You're able to have multiple input files but can only have one output file.
  • Need more consistency in the naming and formatting

The name files was changed to input_files and file was changed to output_files. The name url in files and location in file were also changed to path. You can now also specify multiple files for the output files. An example is the following:

input_files:
  - name: "animal.txt"
    path: "media/workdir/animal.txt"
  - name: "human.txt"
    path: "media/workdir/human.txt"
output_files:
  data: 
    - content: "lion" 
      path: "media/workdir/animal.txt" 
    - content: "tim"
      path: "media/workdir/human.txt"
  oracle: ....
output_files:
  - content: "animal" 
    path: "media/workdir/animal.txt" 
  - content: "humant"
    path: "media/workdir/human.txt"

You can still only specify paths in the content section of output files.
We can distinguish between actual content and the path the a file that contains it by using !path.
An example is the following:

output_files:
  data: 
    - content: !path "animal.txt" 
      path: "media/workdir/animal.txt" 
    - content: "Humans can make music and a warm meal"
      path: "media/workdir/human.txt"
  oracle: ....

So content will now expect the actual content by default and not a path to load it from.

For the feedback, it's all still a bit fuzzy because right now all the content is dumped after eachother. Potential solution:

  • Usage of tabs in the solutions
  • Only show the names and show content when clicking on them.

Most of that will probably need to happen on Dodona itself.

Stdin, Stdout and Stderr

How things currently work, you have to specify the full contents of the stdin, stdout and stderr channels. This can get ugly, when that's a lot of text. This is why the usage of files is also very benificial here.

Example for Stdin:

stdin: !path "media/workdir/animal.txt"

The usage of !path is also present here. This is consistent with what was discussed above.

Under the hood, Stderr and Stdout are both just textual output channels. So they both have the exact same functionality.
If they are a dictionary, they used to expect the key data, but now you can also use content which is more consistent with the rest. Just like before you can also use !path to specify that you want to use a file instead of directly specifying the content.

Examples for Stdout:

stdout: !path "media/workdir/animal.txt"

stdout: 
  content: !path "media/workdir/animal.txt"
  config: ...
  oracle: ...

@BrentBlanckaert BrentBlanckaert marked this pull request as ready for review February 25, 2025 16:20
@jorg-vr
Copy link
Contributor

jorg-vr commented Feb 26, 2025

First review based on the text.

I have a feeling the naming scheme is still a bit inconsistent.

The input_files seems quite logical:

input_files: List of files
  - name:  The name of the file to be used in the code (eg to pass to a function)
    path: The path to the content of the file in the exercise directory

But output_files have me confused.
You specify path and content for each file, but it is completely unclear to me how you can have both at the same time.

What I would have expected based on seeing input_files:

output_files: List of files
  - name: The name of the file the student should write it's output in
    path: The path of the expected output file
    content: Alternative to path, specify the expected content of the file inline here

It would even be great if this content variable was also available for input_files.

Instead path and content are specified at the same time, no name variable is present. And content seems to contain either a filename, the actual content of the file or an explicit path using the !path tag.

I like the !path tag option for content, and it works well and consistent for stdin and stdout, but I really don't see how we can the specify path at the same time. It confuses me and will thus probably also confuse our users.


I left out the oracle case in the above example, as that looks fine.

Copy link
Contributor

@jorg-vr jorg-vr left a comment

Choose a reason for hiding this comment

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

I didn't specify it every time it is relevant, but how to properly handle the name changes seems like the biggest open issue. It will probably be best if we also discus this IRL tomorrow

@@ -39,7 +39,7 @@
}
],
"properties" : {
"files" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly, we'll preferably remain backwards compatible here.
So while we can introduce a new improved naming scheme with input_files and output_files, we'll have to keep supporting the old scheme at the same time.

We could also think about starting to give deprecation notices, and/or automatically updating all existing exercises. (We have done these scripted updates before, but these have to be well timed, and with a growing number of users, we have a growing risk of confusing some, if we don't give proper deprecation warnings)

Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

The code itself looks OK, added some comments here and there.

More high-level:

  • Jorg is right that we need backwards compatibility
  • Your examples are slightly confusing, since the paths in the output_files will not be to the media dir, but rather the name of a file that is either specified in the assignment or as a parameter. So rather something like
output_files:
  data: 
    - content: "lion" 
      path: "animal.txt" 
    - content: "tim"
      path: "human.txt"

And when using a path as "content", it will probably be to a file inside in the evaluation folder of the exercise.

@@ -148,6 +168,7 @@ def _parse_yaml(yaml_stream: str) -> YamlObject:
yaml.add_constructor("!" + actual_type, _custom_type_constructors, loader)
yaml.add_constructor("!expression", _expression_string, loader)
yaml.add_constructor("!oracle", _return_oracle, loader)
yaml.add_constructor("!path", _path_string, loader)
Copy link
Member

Choose a reason for hiding this comment

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

I am not against adding another custom tag, but I do think we need to be slightly careful here, since we also expose all "TESTed types" as tags, this would prevent us from ever having a "path" type in TESTed.

An alternative is working with plain objects, e.g.

- path: "animal.txt" 
  content:
     type: "path"
     path: "media/workdir/animal.txt" 

Again, not against it, just something to consider.

@@ -528,7 +535,7 @@ def get_functions(self) -> Iterable[FunctionCall]:

@define(frozen=True)
class FileUrl:
url: str
path: str
Copy link
Member

Choose a reason for hiding this comment

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

The reason this was named url is that this is a path to the media folder of an exercise, since it is linked on Dodona. The "generated "file" stuff should all have paths to the evaluation folder instead. Now, there is an argument to be made that this distinction is unnecessary, but that is done at the Dodona side (I think, it has been a while, so this should be checked).

But to be clear, I am not against renaming this, just giving some context.

@@ -3,12 +3,20 @@
"""

import math
import os

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'os' is not used.

Copilot Autofix AI 1 day ago

To fix the problem, we should remove the unused import statement for the os module. This will clean up the code and remove the unnecessary dependency, making the code easier to read and maintain.

  • Locate the import statement for the os module on line 6.
  • Remove the line import os from the file tested/oracles/text.py.
Suggested changeset 1
tested/oracles/text.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/tested/oracles/text.py b/tested/oracles/text.py
--- a/tested/oracles/text.py
+++ b/tested/oracles/text.py
@@ -5,3 +5,2 @@
 import math
-import os
 from typing import Any
EOF
@@ -5,3 +5,2 @@
import math
import os
from typing import Any
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Comment on lines +13 to +19
from tested.testsuite import (
FileOutputChannel,
OutputChannel,
OutputFileData,
TextChannelType,
TextOutputChannel,
)

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'FileOutputChannel' is not used.

Copilot Autofix AI 1 day ago

To fix the problem, we need to remove the unused import statement for FileOutputChannel. This will clean up the code and remove unnecessary dependencies, making the code easier to read and maintain.

  • Locate the import statement for FileOutputChannel in the file tested/oracles/text.py.
  • Remove the FileOutputChannel from the import statement on line 13.
  • Ensure that no other parts of the code rely on this import.
Suggested changeset 1
tested/oracles/text.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/tested/oracles/text.py b/tested/oracles/text.py
--- a/tested/oracles/text.py
+++ b/tested/oracles/text.py
@@ -13,3 +13,3 @@
 from tested.testsuite import (
-    FileOutputChannel,
+    
     OutputChannel,
EOF
@@ -13,3 +13,3 @@
from tested.testsuite import (
FileOutputChannel,

OutputChannel,
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants