mathjs icon indicating copy to clipboard operation
mathjs copied to clipboard
verified publisher icon josdejong

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

Open gwhitney opened this issue 3 years ago • 5 comments

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.)

gwhitney avatar Feb 23 '22 07:02 gwhitney

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

gwhitney avatar Aug 17 '22 08:08 gwhitney

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 #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?

josdejong avatar Aug 18 '22 12:08 josdejong

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.

gwhitney avatar Sep 13 '22 00:09 gwhitney

  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.

josdejong avatar Sep 16 '22 09:09 josdejong

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

gwhitney avatar Sep 16 '22 17:09 gwhitney