source-academy / js-slang

Implementations of sublanguages of JavaScript, TypeScript, Scheme and Python
https://source-academy.github.io/source/
Apache License 2.0
64 stars 102 forks source link

Testing Refactor #1707

Open leeyi45 opened 1 month ago

leeyi45 commented 1 month ago

TL; DR

What's going on:

Because our testing utilities relied heavily on the interpreter, removing it means touching almost every single part of js-slang, hence the ridiculous number of lines changed. I've tried to simplify the testing utilities and refactor repetitive code where necessary without breaking anything I hope. For example, I noticed that the stepper's tests were being run as async tests when getEvaluationSteps is no longer asynchronous. Hopefully with these changes it will make such things easier to catch.

I've also configured tsconfig.prod.json to exclude src/utils/testing. Previously this code was being shipped with the rest of js-slang, but given that we only use it for unit testing I don't think it should be included when js-slang is being built.

Given that @martin-henz has indicated that other parts of js-slang are marked for removal, I decided to remove them here to avoid having to refactor the tests for those parts of js-slang.

Please refer to the TODO comments to see what issues need to be addressed before this PR can be merged.

Will resolve these issues:

Some issues that need to be revisited:

leeyi45 commented 1 month ago

This remains a draft because there are some critical questions that need to be answered:

  1. Since svml-machine.ts is removed, we need to specify the behaviour for when runInContext is called with Variant.CONCURRENT. The variant has been left in since there are parts of the vm that still rely on it.
  2. String representations of functions in the cse-machine always default to arrow functions. Is this desired behaviour? Because currently the native runner and the cse-machine have different behaviour.
  3. I am wholly unqualified to update src/__tests__/environment.ts. So the snapshot is failing here but I haven't just gone ahead and changed it.
  4. Stepper doesn't actually do type checking: 0 / "a"; doesn't cause it to throw errors. Does something need to be done about this?
leeyi45 commented 1 month ago

Minor comments: The different runner types have different error messages (and types) for the same problem. For example, stream_length(integers_from(0)); throws a maximum stack size exceeded with the cse-machine (and I think the stepper behaves similarly), while the native runner (which uses the infinite loop detector) throws an InfiniteLoopError with the message Line 1: The error may have arisen from forcing the infinite stream: function integers_from.

I think we need to unify these error messages, but that I think that will have to wait until after this PR is done.

leeyi45 commented 1 month ago

One more thing: I have no idea what inspect() or any of those related things do, so I haven't been able to remove things like the Scheduler

Ideally I'd like to remove things like manualToggleDebugger since it doesn't seem like we're using it anymore