Evaluate meta tests 53 #8

andypants888 · 2022-03-24T03:44:27Z

53 === jira task number

Basic Functionality of the "metaTests "is working now.

The MetaTests
- Are "testing" the code-challenge tests "aka internalTests".
- Are basically pass/fail examples of userCode
- Should see if internalTests are passing/failing correctly

The Pass/Fail detection currently works for only SOME challenges.

The code & output are still messy to read.

Actually I wasn't going to submit a PR yet,
but I wanted to get some feedback on variable names and conventions
and show what my current draft looks like.

PS:
most files are copied/modded from frontEnd test-evaluation files.
pMap had to be downgraded to version 4 for compatibility with require/import on nodejs

because the date fields were undefined they weren't showing up on graphql

…est-validator-37

oriooctopus · 2022-06-21T20:23:17Z

.gitignore

@@ -82,6 +82,7 @@ ssl
 nbproject
 public/uploads/*
 !public/uploads/.gitkeep
+.vscode/


What's the reason for this?

.vscode/launch.json is a config file that gets added to the project from the vscode debugger.

I wanted to keep all vscode config files out of our final repo

oriooctopus · 2022-06-21T20:23:41Z

.gitignore

+############################
+
+src/api/code-challenge/content-types/code-challenge/*.js
+!src/api/code-challenge/content-types/code-challenge/lifecycles.js


Why are these being ignored?

This is my "scalable" way of dealing with the .js files created during typescript compilation.

lifecycles.js had an issue with typescript on strapi. Strapi V4 announced May 12, 2022 they would begin beta support for typescript in this post.

Do you want me to investigate the beta support? Currently we are not using TS for the strapi lifecycles hooks file.

We originally discussed it here:
#8 (comment)

I would love it if you did that.

Also, I don't think the compiled files should go in the same folder as the uncompiled files. I think they should go in adist folder or something. That would automatically solve the gitignore issue as well

I would love it if you did that.

To test the beta version of TypeScript support, run the command npx create-strapi-app@beta my-app (--ts) in a terminal.
I will need to rebuild the app. I will save this for the last part.

Can I restore the previous version using git reset --hard ?

I've decided to wait a month for strapi to continue working out bugs with their beta typescript support

Also, I don't think the compiled files should go in the same folder as the uncompiled files. I think they should go in adist folder or something. That would automatically solve the gitignore issue as well

I agree with the dist folder, but should the dist folder be a clone of the src folder or just a smaller folder within src? Currently very few files are being used with TS, and TS support is still in beta for strapi

I would say a clone of the src folder but let me know if you have issues

I will wait until strapi integrates TS to implement the outDir & srcDir, since I'm not sure if running "strapi start" / "strapi develop" will work with a dist folder yet.

oriooctopus · 2022-06-21T21:28:49Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+// Currently unused function. Did you write this Oliver?
+const getInternalLabel = async (event) => {


Yeah I did. Can you just remove it?

Got it. It's gone now.

oriooctopus · 2022-06-21T21:31:46Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+async function runInternalTests(
+  internalTests,
+  metaCaseCode,
+  challengeLabel,
+  metaTestLabel,
+  metaTestId,
+  evalResultShouldBe,
+  count
+) {
+  let results = await executeTests(
+    metaCaseCode,
+    internalTests,
+    metaTestLabel,
+    challengeLabel,
+    metaTestId,
+    evalResultShouldBe
+  );
+  // MetaTest Final Results being logged to Console.
+  console.log(`<METATEST EXAMPLE SET #${count + 1}>`, results);
+
+  return results;
 }


It seems like all this function does it just run executeTests, why not remove this function and call that directly?

Ok, I got rid of runInternalTests(), and now called executeTests directly

oriooctopus · 2022-06-21T21:33:46Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

-  return internalTests;
+
+  // Rename internalTest to internalTestCode
+  internalTests.map((internalTestPackage) => {


why do you need to have internalTestCode instead of internalTest? It seems unnecessarily confusing

during the error output:

and source code:

internalTest causes some confusion between internalTestCode vs internalTestLabel vs internalTestId inside the code

Originally, internalTest === internalTestCode

I left a comment explaining why this renaming was needed in the code.

We could later rename "internalTest" --> "internalTestCode" in our database to clean up this code.

oriooctopus · 2022-06-22T16:00:26Z

src/api/code-challenge/content-types/code-challenge/utils.ts

+        if (evaluateWithContext(codeWithCondition, context)) {
+          return {
+            index: i,
+            passed: true,
+          };
+        }
+      }


I honeslty think it'd be a bit more intuitive to read if you put evaluateWithContext(codeWithCondition, context) in a variable

This is yours, but I tried to apply your suggestion.

yeah I'm not setting the best example sorry.

Although as far as examples go, my rushed code is still decent. Just not as good as my normal stuff

oriooctopus · 2022-06-22T16:01:34Z

tsconfig.json

@@ -0,0 +1,101 @@
+{
+  "compilerOptions": {


Can you remove the commented out code

oriooctopus · 2022-06-22T16:02:43Z

src/api/code-challenge/content-types/code-challenge/general.ts

@@ -0,0 +1,50 @@
+// andy - Not Directly Used in MetaTests


I'm a bit confused. Why is this file added if it isn't used in metaTests? Is it just apart of the general migration to TS?

this file is imported into:

lifecycles.js - const { removeEmpty } = require("../code-challenge/general");

utils.ts import { getErrorMessage } from "../code-challenge/general";

I would remove the comment then. Also, can you remove any unused functions and commented code?

oriooctopus · 2022-06-22T16:06:25Z

src/api/code-challenge/content-types/code-challenge/test-evaluator.ts

+  };
+
+  const setNullError = () => {
+    evalError = {


I don't like this thing you're doing here and in the above function. Clean javascript is usually according to a functional paradim, meaning that you generally pass in something and get something out. Here you are mutating something, which leads to things getting confusing. Just return a new object, and use that value to reassign the original variable. That might seem the same but it's cleaner to reassign at the outer level rather than within the function... it's more relevant. Let me know if you don't follow

Ok I tried to clean it up. Let me know if you want more changes.

oriooctopus · 2022-06-22T16:08:23Z

src/api/code-challenge/content-types/code-challenge/test-evaluator.ts

+  } finally {
+    determineReturnObject();
+  }
+  restoreConsoleLog();
+  return {
+    error: evalError,
+    userPassed: userPassed,
+    description: descriptionMessage,
+    resultType: typeof result,
+    evalResult: result,
+  };


This finally statement doesn't seem necessary. Rather than reassigning why not just fold it into the final return statement?

I don't understand. What do you mean by reassigning?

determineReturnObject reassigns values and then you finally returns the reassigned variables (this by the way is an example of how the non-functional paradigm is making things confusing. Because when you see determineReturnObject it's not intuitive that that causes variable reassignments

Ok I tried to combine the var assignment with the return statement.

The vars need to be assigned after the try...catch block

oriooctopus

getting closer!

oriooctopus · 2022-06-25T19:20:08Z

src/api/code-challenge/content-types/code-challenge/curriculum-helpers.ts

@@ -0,0 +1,30 @@
+// @ts-expect-error will fix later


Can you remove whichever functions are unused?

oriooctopus · 2022-06-25T19:20:41Z

src/api/code-challenge/content-types/code-challenge/general.ts

@@ -0,0 +1,50 @@
+// andy - Not Directly Used in MetaTests


I would remove the comment then. Also, can you remove any unused functions and commented code?

oriooctopus · 2022-06-25T19:21:20Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js


-// runMetaTestsNeed more specific name later, maybe use "validateInternalTests"
-async function runMetaTests(eventTests, eventMetaTests) {
+const conductMetaTestsOnTests = async (


yeah I think runMetaTests would be a much better name. Why did you get rid of it?

I changed back to runMetaTests. I didn't think it was descriptive enough. I am getting the feeling the naming conventions are somewhat subjective.

Is there some standard I can read on naming conventions? They exist in other fields like chemistry etc. Nomenclature?

oriooctopus · 2022-06-25T19:22:29Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+    async (internalTest) => {
+      const {
+        label: internalTestLabel,
+        internalTestCode: internalTestCode,


you can just do internalTestCode, you don't need internalTestCode: internalTestCode

Thanks. I changed it.

oriooctopus · 2022-06-25T19:25:07Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+      return {
+        short: {
+          description: null,
+          ...pick(testEvaluatorResults, ["description"]),
+        },
+        long: {
+          challengeLabel,
+          metaTestId,
+          internalTestId,
+          internalTestLabel,
+          userPassed: null,
+          evalResultType: null,
+          ...pick(testEvaluatorResults, [
+            "evalResultType",
+            "userPassed",
+            "evalError",
+          ]),
+        },
+      };


I would do

return { message: testEvaluatorResults.description, debuggingInfo: { challengeLabel, metaTestId, internalTestId, internalTestLabel, userPassed: testEvaluatorResults.userPassed, evalResultType: testEvaluatorResults.evalResultType, evalError: testEvaluatorResults.evalError, } };

I don't understand why you're double defining things with null the first time.

Thanks for the tip.
Is null better than undefined? I was under the impression it was better to have values be null vs undefined.

oriooctopus · 2022-06-25T19:31:15Z

src/api/code-challenge/content-types/code-challenge/test-evaluator.ts

+  let evalResult: unknown;
+  let evalError: unknown;
+  const formattedCode = getCode(metaCaseCode, removeComments);
+  const logs = [] as Array<unknown>;


I was to lazy to fix this. Can you make it not unknown?

Ok, I changed it to string.

oriooctopus · 2022-06-25T19:32:03Z

src/api/code-challenge/content-types/code-challenge/test-evaluator.ts

+  }
+
+  restoreConsoleLog();
+  return assignReturnObject();


when a function gets something, aka returns something, then you prefce it with get. Here you'er implying that you're reassinging within the function instead of getting something

ok thanks. I changed the name to getMetaTestResultObject()

oriooctopus · 2022-06-25T19:32:16Z

src/api/code-challenge/content-types/code-challenge/utils.ts

@@ -0,0 +1,156 @@
+// Import filepaths changed from original


don't understand

I don't either. I'm not sure I wrote this.

I'll remove all the commented / unused functions

oriooctopus · 2022-06-25T19:32:34Z

src/api/code-challenge/content-types/code-challenge/utils.ts

+    }: {
+      condition: string;
+    }) => {
+      debugger;


Can't, it is currenty used in test-evaluators.ts

import { evaluateWithContext, getCode, getEvaluationContext, overrideConsoleLog, restoreConsoleLog, } from "../code-challenge/utils";

debugger is?

oriooctopus · 2022-06-25T19:33:14Z

src/api/code-challenge/content-types/code-challenge/utils.ts

+  const context = getEvaluationContext(code);
+  const logs = [] as Array<string>;
+  // Disabled for debugging test-evaluator
+


nit remove extra newline

they are being added via auto-format, should that be disabled?

oriooctopus · 2022-06-30T21:42:26Z

.gitignore

+############################
+
+src/api/code-challenge/content-types/code-challenge/*.js
+!src/api/code-challenge/content-types/code-challenge/lifecycles.js


I would say a clone of the src folder but let me know if you have issues

oriooctopus · 2022-06-30T22:35:13Z

src/api/code-challenge/content-types/code-challenge/general.ts

+  if (error instanceof Error) return error.message;
+  return String(error);
+};


In terms of formatting I would add a newline before the return statement

oriooctopus · 2022-06-30T22:37:00Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+const runMetaTests = async (eventParams) => {
+  const { eventTests, eventMetaTests, challengeLabel } = eventParams;


why not just do

const runMetaTests = async ({ eventTests, eventMetaTests, challengeLabel }) => {

changed it.

oriooctopus · 2022-06-30T22:37:13Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

  const [internalTests, metaTests] = await Promise.all([
    getInternalTests(eventTests),
    getMetaTests(eventMetaTests),
  ]);
+  for (let i = 0; i < metaTests.length; i++) {


Always have a newline before a for loop and a return statement, unless they are the first line in a function

Thanks. I've added newlines in other PR files for this suggestion too.

oriooctopus · 2022-06-30T22:45:43Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+    let metaTestResult = await executeTests(
+      metaTests[i].caseCode,
+      internalTests,
+      metaTests[i].label,
+      challengeLabel,
+      metaTests[i].id,
+      metaTests[i].passes
+    );


When you get 3 or more parameters, especially this amount, it's a good idea to have just one parameter, an object with properties. The benefit is that the order isn't important and the parameters have labels. With this many parameters it's hard to keep track of things

I think this is a repeat of the comment below

oriooctopus · 2022-06-30T22:47:06Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+        message: {
+          description: testEvaluatorResults.description,
+        },


this doesn't need to be an object, it can just be

message : testEvaluatorResults.description

Thanks for the catch.

oriooctopus · 2022-06-30T22:47:53Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  /* Originally, the content of internalTest === internalTestCode in strapi.
+ I renamed internalTest to internalTestCode for clarity in lifecycles.js & test-evaluator.ts
+ This renamed variable makes: (internalTestCode vs internalTestLabel vs internalTestId) the children of internalTestPackage */


When you do multi line comments you line it up like:

/** * wefweewfe * wfwefw af awe */

Notice the * at the beginning of each line

Thanks for the tip. I changed the *'s

Should I do this for 2+ lines or 3+ lines long multi-line comments?

oriooctopus · 2022-06-30T22:50:22Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+    .map((internalTestPackage) => {
+      internalTestPackage.internalTestCode = internalTestPackage.internalTest;
+      delete internalTestPackage.internalTest;
+      return internalTestPackage;
+    })


don't delete and add properties, return a new object:

=> ({ internalTestCode: internalTestPackage.internalTest, ...omit(internalTestPackage, 'internalTest') })

but also, why are you calling it internalTestPackage? Just call it internalTest. It is confusing to have all these arbitrary renamings. The less renaming, the easier it is for someone to keep track of things

oriooctopus · 2022-06-30T22:56:52Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+ This renamed variable makes: (internalTestCode vs internalTestLabel vs internalTestId) the children of internalTestPackage */
+  return internalTests
+    .map((internalTestPackage) => {
+      internalTestPackage.internalTestCode = internalTestPackage.internalTest;


Instead of the big comment there I would just add a comment above this line saying:

renaming the property from `internalTest` to `internalTestCode` for clarity

That's all you need, the rest just adds more to read. Less is more

Ok. I added in your abridged comment

oriooctopus · 2022-06-30T23:02:41Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+        label: internalTestLabel,
+        internalTestCode,
+        id: internalTestId,
+      } = internalTest;
+
+      // console.log("<internalTest pMap> ", internalTest);
+      // console.log("newTest name: ", newTest);
+
+      const testEvaluatorResults = await runTestEvaluator({
+        metaCaseCode,
+        internalTestCode,
+        metaLabel,
+        challengeLabel,
+        metaTestId,
+        internalTest,
+        evalResultShouldBe,
+      });


So thinking about internalTest vs internalTestCode more I have two thoughts:

The most clear thing to do would be to probably rename the property internalTest to just, code, in the strapi backend. However that's a bit time consuming so it's not worth doing.

It seems like the only time where internalTest isn't the best property name is here, because we already have a variable called internalTest. I would say that if you don't destructure here, then you don't have that issue. That's not the most ideal, but it's the lesser of two evils and it's pretty common to avoid awarkdness like this. I mean, renaming a property is more awkward and confusing than that. So yeah just to reiterate, it's ok to avoid desctructuring to avoid awkwardness.

And yeah, just get rid of the renaming and don't desctructure here

Ok I got rid of the renaming structure, but struggling to remove the variable that stores internalTest.internalTest, since I can't pass in

{ internalTest.internalTest: "string here" // error! }

or rename internalTest = internalTest.internalTest without wiping out the object.

Let me know what you think.

I left the declarations regular for clarity.

oriooctopus

approved with a couple comments

oriooctopus · 2022-07-06T00:50:56Z

src/api/code-challenge/content-types/code-challenge/utils.ts

+    }: {
+      condition: string;
+    }) => {
+      debugger;


debugger is?

oriooctopus · 2022-07-07T19:15:56Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  const eventParams = {
+    eventTests: event.params.data.tests,
+    eventMetaTests: event.params.data.MetaTest,
+    challengeLabel: event.params.data.internalLabel,
+  };
+  runMetaTests(eventParams);


you don't need to have a separate line here, just do

runMetaTests({ eventTests: event.params.data.tests, eventMetaTests: event.params.data.MetaTest, challengeLabel: event.params.data.internalLabel, });

Thanks for the catch.

oriooctopus · 2022-07-07T19:16:23Z

src/api/code-challenge/content-types/code-challenge/test-evaluator.ts

+  const getNoError = () => {
+    return "no eval() error detected";
+  };


This doesn't need to be a function it can just be a string

Thanks again.

oriooctopus · 2022-07-07T19:17:43Z

src/api/code-challenge/content-types/code-challenge/test-evaluator.ts

+      case typeof evalResult !== "boolean" && typeof evalResult !== "string":
+        return {
+          evalError: evalError ? getShortError() : getNoError(),
+          userPassed: false,
+          description: `FAIL: EvalResultType should always be 'boolean' but is currently '${typeof evalResult}'.`,
+          evalResultType: typeof evalResult,
+          evalResult: evalResult,
+        };


I would honestly just put this as the last 'default' case but that might just be a matter of opinion

I think that makes sense. I changed it.

oriooctopus and others added 30 commits February 1, 2022 16:41

Improve env variable handling for db connections

5f985e6

Add getNextLessonSlug query

9819671

upgrade stapi

e671771

Fix sorting order bug

2aacbcf

because the date fields were undefined they weren't showing up on graphql

adjust default pagination limit

ce4265a

feature: Query Test & MetaTest internals

293e03e

Strapi setup walkthrough

e01ce01

README updated with Markdown

bdfe150

feature: add findMany for multiple tests/metas

136b06a

feature: cleaned up lifecycle hooks

1ffb1a7

updated README

3bf8d62

Added database setup

d7b84ae

Added git workflow

c07f390

small changes after feedback

ea062ce

Merge pull request oriooctopus#2 from bjornrl/update-readme

5ff2f5f

Changed npm to yarn + small fixes

304f06d

Merge pull request oriooctopus#5 from bjornrl/update-readme

a488e41

Merge branch 'master' of https://github.com/oriooctopus/acc-v4 into t…

72f687e

…est-validator-37

feature: edits to pr#3 , 37-jira-task

3352422

feature: pr oriooctopus#3 edits round2

e55605a

Merge pull request oriooctopus#3 from andypants888/test-validator-37

d312b00

Fix bug on publish of lesson

f3963e9

Switch 'challenges' to 'steps'

83a52bf

Readd playground type

64032d4

backups

d088571

modify challenges

8cc3cfa

Strapi plugin config sync (oriooctopus#4)

eb20f83

Update README.md

97988af

buggy, working metaTests

c0d619d

debugged max call stack error

8688738

andypants888 added 3 commits June 20, 2022 14:28

save before moving switch-case to own function

afdfc87

moved switch-case to determineCase function

d0cf43b

cleaned up PR draft 1

21e6854

oriooctopus requested changes Jun 22, 2022

View reviewed changes

andypants888 added 12 commits June 23, 2022 13:26

restore: before removing runInternalTests()

2ad628d

removed runInternalTests() & reoganize output

1237dd0

simplify error output

86236c9

divide output into short & long, add comments

2d879e9

combine map & sort

35114f2

move compareIds() to utils

91c5290

fix naming & comments

a2d6076

pre-lunch save

cff20cb

simplify determineReturnObject() logic

d999483

address comments about var names & reassignment

76fe019

addressed utils.ts comments

6c8b517

address ts compiler errors

428ddd7

oriooctopus requested changes Jun 25, 2022

View reviewed changes

andypants888 added 3 commits June 27, 2022 15:41

after address comments

b1dedd3

double-checked comments, pending review

a8bfae1

strip-comments not previously installed

88a10c4

oriooctopus requested changes Jul 1, 2022

View reviewed changes

andypants888 added 6 commits July 5, 2022 16:07

save: before edit internalTest -> internalTestID

599be10

save: before rename internalTestCode --> internalTest

b1b561d

almost done renaming internalTestCode

b0f0731

remove renaming function

916515e

save: before making dist folder

c9dc21b

final touchs metaTests

fed7519

oriooctopus approved these changes Jul 7, 2022

View reviewed changes

final comments before merge

f1e1f31

oriooctopus force-pushed the master branch from 35c3218 to db0d7eb Compare July 31, 2022 15:42

		// Currently unused function. Did you write this Oliver?
		const getInternalLabel = async (event) => {

		const runMetaTests = async (eventParams) => {
		const { eventTests, eventMetaTests, challengeLabel } = eventParams;

Evaluate meta tests 53 #8

Are you sure you want to change the base?

Evaluate meta tests 53 #8

Conversation

andypants888 commented Mar 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andypants888 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andypants888 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oriooctopus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andypants888 Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andypants888 Jun 23, 2022 •

edited

Loading

andypants888 Jun 23, 2022 •

edited

Loading

andypants888 Jul 5, 2022 •

edited

Loading