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

test: Check return code, stdout, stderr of (almost) every example #2447

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

gwhitney
Copy link
Collaborator

Runs each of the files ending in '.js' in the examples directory or
a subdirectory thereof via node. Verifies the return code is normal
for each one. Compares stdout and stderr to reference copies.

As requested in #2264 (hopefully this can be a first step in clearing that and/or #2279 out of the PR queue).
Some caveats that should be considered in review of this PR:

  1. I did not check that the results of all of the examples were sensible; if they ran, I simply took their current output as the reference.
  2. Please someone check that I fixed the non-operational examples/advanced/custom_argument_parsing.js in the correct way (it's quite unclear to me how one is supposed to create a subscope in user code (as opposed to internally within mathjs, where it is easy to import utils/scope.js).) Do I even need to make a copy of the scope I get in the raw function, as the document says it's already a shallow copy of the ambient scope? (i.e. can I set the specified variable in scope and dispense with the creation of fnScope altogether? I verified that it produced the same values if I did it that way, but I wasn't sure it was truly "safe" to the point where it's OK for the example to show doing things that way.)
  3. node examples/advanced/custom_loading.js didn't actually run at all, since by default node doesn't support import. So I renamed this file to custom_loading.mjs (as it seems to be a module anyway) and then it's ignored by the test harness I have added. Problem solved ;-) Seriously, if there's a way to add a custom test for this one in examples.test.js, please guide me. Thanks.
  4. There is an issue with examples/import.js. I assumed that on the automated test machine, numbers and numeric would not be installed. So I used as the reference the output when these are not installed. That's all fine, but it means importing these external packages is not tested. If it should be, what should I do? I didn't think that having a test install a package would be a good idea, as it will then be too easy for these packages to end up checked in as dependencies of mathjs, which they are not. So should I just leave this be or should we try to engineer a specific test for importing external packages into mathjs?
  5. I didn't see any convenient way to test examples/advanced/web_server but thats a sub-sub-directory and so ignored by my harness. Again problem solved ;-) and again if there is a suggestion on a custom test for this stuff, please let me know.
  6. I didn't even try to come up with a way to test the things in examples/browser. There are no .js files so the harness doesn't care. If there is a reasonable way to test them, let me know and I will add another harness just for them.

Anyhow, these comprise a really solid putting of mathjs through its paces, so hopefully what's there in this PR will be helpful in the future. (I am thinking also of writing a test in a separate PR which checks the output of every example in every embedded-docs help against a reference copy as well, as that should be pretty complete, too. There's a fair amount of overlap there with the unit tests, but also a fair amount of unique examples, and so it seems like we may as well use them.)

…mple

  Runs each of the files ending in '.js' in the examples directory or
  a subdirectory thereof via `node`. Verifies the return code is normal
  for each one. Compares stdout and stderr to reference copies.
@gwhitney
Copy link
Collaborator Author

Happy to update this to latest mathjs develop, but need guidance on the many questions above.

@josdejong
Copy link
Owner

josdejong commented Aug 18, 2022

I'm not sure how I missed this PR before, sorry for not looking into it before.

  1. I think this is a good start. In a second interation, we can store a snapshot of the correct output and compare against that if we want.
  2. Your change works I think, though this is probably not the most efficient as we copy all contents from scope into fnScope, and also, if we would change something in scope afterwards, it's not reflected in fnScope. It would be nice to implement a new MultiMap(fnScope, scope) utility which reads values iterating over all provides scopes, and writes values into the first provided scope.
  3. Renaming to .mjs makes sense. The example working out of the box requires changing the import from '../../' to '../../lib/esm/index.js', I've fixed that example in develop now (see also Custom bundling example broken #2144).
  4. I can look into why this example fails to run, I'm not sure. Maybe because of the require(...) statements halfway the file inside a try/catch block? (if that is the reason, maybe await import(...) could help out).
  5. Yes, please ignore this one
  6. We would have to open these examples in a headless browser. That may require quite some work. Let's address that separately ok to keep this PR limited?

@gwhitney
Copy link
Collaborator Author

Sorry about the delay in getting back on this, I've been focused on Pocomath and/or the TypeScript experiments.

  1. Well, this commit does include snapshots of the output of each of the examples that ran. But I did not look through them manually to make sure they are all as desired. Maybe someone should before we commit them as the "correct" output?
  2. My apologies, but I am confused by your response here. What/how could scope change so that it wouldn't be reflected in fnScope? Should I leave the code as is, or should I change it to just set x in scope rather than bothering to make the fnScope copy?
  3. I will take another look at this one on top of current develop and see if I can make it a working test.
  4. It doesn't fail to run, it just doesn't do much because neither 'numbers' or 'numeric' is installed. I didn't see how it was safe for the test to install modules, because then I thought they might get checked in to the mathjs repository, etc. Any suggestions on how to make this a test with real behavior, or should I just skip this one?
  5. Check.
  6. Check.

Thanks for further guidance.

@josdejong
Copy link
Owner

  1. Ok I'll look though all the outputs to double check
  2. Hmmm, that is a good point. It can indeed not happen that there are changes made in scope that are not reflected in fnScope since the map is copied with every execution of the function. Ok nevermind, it's maybe not the most efficient solution but let's not over engineer this example and just keep it as is with your onst fnScope = new Map(scope).
  3. 👍
  4. I would say let's just skip the example for now.

@gwhitney
Copy link
Collaborator Author

OK, I think we are clear on all of the points. I will try to move this back toward being merge-ready.

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