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 TypeScript #553

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

Conversation

BrentBlanckaert
Copy link
Collaborator

@BrentBlanckaert BrentBlanckaert commented Oct 23, 2024

No description provided.

@BrentBlanckaert
Copy link
Collaborator Author

No modules are recognized. I suspect this has to do with things missing in the flake file. I have no idea why it can't find certain modules like fs or typescript. I added pkgs.typescript to my dependecies so I would atleast think typescript would be supported.

@BrentBlanckaert
Copy link
Collaborator Author

BrentBlanckaert commented Oct 26, 2024

I was able to solve the problem. I get no errors anymore when running

nix develop
tsx solution.ts

solution.ts:

import * as fs from "fs";
import * as ts from "typescript";

const GLOBAL_VAR = "GLOBAL";

However when I run the following, I still face the same problem

nix develop
python -m tested -c exercise/simple-example/config.json

The configuration pointing to the same solution file and test suite is the same as in the global test.

@BrentBlanckaert
Copy link
Collaborator Author

BrentBlanckaert commented Oct 27, 2024

When running anthing with tsx, I no longer get an error regarding unknown modules.
However for the test_batch_compilation_no_fallback_runtime test I was forced to use tsc in the compilation function.
Because of this change, other tests are failing again. tsc is suffering from the same problem where it can't find certain modules like fs and typescript. This also links back to the problem in my previous comment.

@BrentBlanckaert
Copy link
Collaborator Author

None of these are wrong for me. I don't get why tests are complaining about csharp.

@jorg-vr
Copy link
Contributor

jorg-vr commented Nov 21, 2024

The test complains about FileNotFoundError: [Errno 2] No such file or directory: 'dotnet'

So it probably has something to do with the docker image. This is the C# part:

# C# dependencies
curl https://packages.microsoft.com/config/debian/11/packages-microsoft-prod.deb --output packages-microsoft-prod.deb
dpkg -i packages-microsoft-prod.deb
rm packages-microsoft-prod.deb
apt-get update
apt-get install -y --no-install-recommends dotnet-sdk-8.0

Some ideas

Potential cause: maybe the curl or apt-get install had connection issues when building the image. Resulting in a bad dotnet install. As the docker is reused instead of rebuilt as long as no changes are applied this might persist the bug.
Potential solution: make some changes to the docker, to trigger a rebuild.

Potential cause: apt-get install -y --no-install-recommends dotnet-sdk-8.0 install a different version, potentially on a different location
Potential solution: Google about changes to dotnet, with the error

Potential cause: dotnet is not properly added to the PATH
Potential solution: Either figure out why it is no longer added to PATH and/or add it manually

Potential cause: Your added commands happening right before the dotnet install, make dtnet install fail
Potential solution: Switch positions in docker file

@BrentBlanckaert
Copy link
Collaborator Author

The test complains about FileNotFoundError: [Errno 2] No such file or directory: 'dotnet'

So it probably has something to do with the docker image. This is the C# part:

# C# dependencies
curl https://packages.microsoft.com/config/debian/11/packages-microsoft-prod.deb --output packages-microsoft-prod.deb
dpkg -i packages-microsoft-prod.deb
rm packages-microsoft-prod.deb
apt-get update
apt-get install -y --no-install-recommends dotnet-sdk-8.0

Some ideas

Potential cause: maybe the curl or apt-get install had connection issues when building the image. Resulting in a bad dotnet install. As the docker is reused instead of rebuilt as long as no changes are applied this might persist the bug. Potential solution: make some changes to the docker, to trigger a rebuild.

Potential cause: apt-get install -y --no-install-recommends dotnet-sdk-8.0 install a different version, potentially on a different location Potential solution: Google about changes to dotnet, with the error

Potential cause: dotnet is not properly added to the PATH Potential solution: Either figure out why it is no longer added to PATH and/or add it manually

Potential cause: Your added commands happening right before the dotnet install, make dtnet install fail Potential solution: Switch positions in docker file

I just tried all of the above. Nothing seems to work. I also didn't find anything about new changes that could of broken it.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 54 out of 68 changed files in this pull request and generated 2 suggestions.

Files not reviewed (14)
  • .devcontainer/dodona-tested.dockerfile: Language not supported
  • tested/dsl/schema-strict.json: Language not supported
  • tested/dsl/schema.json: Language not supported
  • tests/exercises/echo-function-additional-source-files/workdir/echo.ts: Evaluated as low risk
  • tested/internationalization/nl.yaml: Evaluated as low risk
  • tested/languages/language.py: Evaluated as low risk
  • tested/languages/init.py: Evaluated as low risk
  • tested/testsuite.py: Evaluated as low risk
  • tested/languages/typescript/config.py: Evaluated as low risk
  • tests/exercises/echo-function-file-input/solution/correct.ts: Evaluated as low risk
  • tests/exercises/counter/solution/solution.ts: Evaluated as low risk
  • tests/exercises/counter/solution/solution-eslint.ts: Evaluated as low risk
  • tested/languages/typescript/generators.py: Evaluated as low risk
  • tested/languages/typescript/parseAst.ts: Evaluated as low risk
Comments skipped due to low confidence (5)

tested/languages/typescript/linter.py:84

  • Ensure end_row is not None before performing subtraction to avoid TypeError.
rows = end_row - start_row if end_row and end_row > start_row else None

tested/languages/typescript/linter.py:87

  • Ensure end_col is not None before performing subtraction to avoid TypeError.
cols = end_col - start_col if end_col and end_col > start_col else None

tests/exercises/echo-function-additional-source-files/solution/correct.ts:1

  • The @ts-ignore comment should be justified or removed if not necessary.
// @ts-ignore

tests/exercises/echo-function-additional-source-files/solution/correct.ts:4

  • The content parameter should have a type annotation.
function echo(content) {

tests/exercises/echo-function-file-input/solution/correct-async.ts:3

  • The parameter 'content' should have a more specific type than 'any'. It should be 'string'. Also, the return type of the function should be 'Promise'.
function echoFile(content: any) {

tested/languages/typescript/linter.py Outdated Show resolved Hide resolved
tested/languages/typescript/templates/values.ts Outdated Show resolved Hide resolved
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.

Looks good, I have two minor nitpicks left

tested/languages/language.py Outdated Show resolved Hide resolved
// Temporarily allow objects with "message" and "name".
// TODO: remove this once the semester is over
// noinspection PointlessBooleanExpressionJS
if (typeof exception === 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

I would remove this special case: it was added to not change the behaviour of JavaScript judging in the middle of the course, so it can probably be removed from JavaScript (and no need to add it here to TypeScript)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What should I do about those tests that depend on them?

Copy link
Member

Choose a reason for hiding this comment

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

I would update the submissions that are being tests to account for the new behaviour (the PR that introduced this is #361, which might be helpful): all exceptions must inherit from Error.

It is possible that some example solutions in the js repo will need to be updated as well. In that case, I would not remove this for JS in this PR, but create an issue for this and remove the special case in JS in another PR (this one is big enough as it is)

@BrentBlanckaert
Copy link
Collaborator Author

Okay, I think all we have to do is change the docker for Dodona before merging.

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.

Looking good.

Only some minor notes/questions left.

A pr to update the docker image will be made automatically after merging this one. So no need to worry about that

@@ -114,5 +118,6 @@ EOF

USER runner
WORKDIR /home/runner/workdir
RUN dotnet --version
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be a no-op
Is this an essential part to fix the dotnet issues in the container?
If so could you explain why?

else:
prefix = ""
return (
f"{prefix}{statement.variable} = "
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that the typescript judge does not type it's variables. (I think this is the place where types should be added)

I would not add it in this pr, as it is working as is and adding types can be a hassle. But I would assume as a user that the statements rendered by the judge would be typed.
So it might be relevant to create an issue to remember this for the future and add it in a future pr


# TypeScript dependencies
npm install -g [email protected] [email protected]
npm install -g @types/node @typescript-eslint/parser @typescript-eslint/eslint-plugin --save-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think --save-dev should be added here. Eslint is used in production by the linter function you wrote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants