Break out tracer-config.ts to its own file #158

robertbrignull · 2020-08-24T11:58:21Z

Pulls a bunch of code from setup-tracer.ts to a new tracer-config.ts file. This is the code that generates the environment variables to be set when running later processes so that CodeQL can intercept the compiler commands.

This is in preparation for chaning the behaviour when in CLI mode to output the env vars to a file instead of using core.exportVariable, while keeping all the code that generates the environment the same. Also while I'm doing it I've written tests for this code which was previously untested.

Also reverts a bad change from a previous PR of mine. In https://github.com/github/codeql-action/pull/140/files#diff-244be00bfdf0600dd7b228caba9f8232L57 I changed the type of the argument from an object to an array because I thought it wouldn't make a difference, but I missed a call to Object.keys which then silently did the wrong thing of returning keys like 0, 1, 2, 3... This basically didn't make a difference thankfully so tracing still worked, and the only functionality that was broken by this was the requirement to put the spec from cpp at the end.

Other than the above noted fix of a regression, this PR shouldn't make any changes to behaviour.

@sampart do you have scope to review this?

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.

sampart

Thanks for this refactor - this is a good step forward.

Many of my comments are about the tests; I think it's important that tests act a bit like documentation (since by nature they describe the system's behaviour), or at least that they make sense without much context (as someone may have to fix a failing test without knowing the project well), hence my insistence on good names.

sampart · 2020-08-25T07:44:34Z

src/setup-tracer.ts

+          { env: { 'ODASA_TRACER_CONFIGURATION': tracerConfig.spec } });
      }
+
+      // NB: in CLI mode these will be outputted to a file instead of exported with core.exportVariable


Suggested change

// NB: in CLI mode these will be outputted to a file instead of exported with core.exportVariable

// NB: in CLI mode these will be output to a file rather than exported with core.exportVariable

sampart · 2020-08-25T07:45:32Z

src/tracer-config.test.ts

+// A very minimal setup
+test('tracerConfig - minimal', async t => {


Suggested change

// A very minimal setup

test('tracerConfig - minimal', async t => {

test('tracerConfig - minimal setup', async t => {

sampart · 2020-08-25T07:47:33Z

src/tracer-config.test.ts

+        'SEMMLE_DEPTRACE_SOCKET': 'SEMMLE_DEPTRACE_SOCKET',
+        'SEMMLE_JAVA_TOOL_OPTIONS': 'SEMMLE_JAVA_TOOL_OPTIONS',
+        'CODEQL_VAR': 'CODEQL_VAR',
+      }


What's happened to foo here?

foo is not listed here because it's already in the environment with a different value. That's the point of this test but I agree it's very unclear. I'll add a comment.

sampart · 2020-08-25T07:50:45Z

src/tracer-config.test.ts

+    process.env['CODEQL_VAR'] = 'abc';
+
+    const result = await tracerConfig(codeQL, config, Language.javascript);
+    t.deepEqual(result, {


I don't understand this assertion - it looks like no variables have changed.

In particular, the comment above says // Existing vars should not be overwritten, unless they are critical or prefixed with CODEQL_, but CODEQL_VAR is unchanged, as are the critical vars. What am I missing?

The difference is that foo is missing, and "overwritten" here refers to the values from the CodeQL call overwriting those that exist in the environment already. I've add some comments and swapped setting process.env and setting up the mocked CodeQL response incase that makes it easier to see.

sampart · 2020-08-25T07:51:54Z

src/tracer-config.test.ts

+  });
+});
+
+test('concatTracerConfigs - minimal', async t => {


Suggested change

test('concatTracerConfigs - minimal', async t => {

test('concatTracerConfigs - minimal configs correctly combined', async t => {

sampart · 2020-08-25T08:16:10Z

src/tracer-config.test.ts

+      },
+    });
+
+    t.deepEqual(undefined, await getTracerConfig(config, codeQL));


Suggested change

t.deepEqual(undefined, await getTracerConfig(config, codeQL));

t.deepEqual(await getTracerConfig(config, codeQL), undefined);

sampart · 2020-08-25T08:16:24Z

src/tracer-config.test.ts

+  });
+});
+
+test('getTracerConfig - full', async t => {


full what? And when that thing is full, what behaviour are you expecting?

I've renamed to "valid spec file". The name "full" came from the fact that we've tested each function individually and now we're testing them all at once by calling the outer function.

src/tracer-config.ts

sampart · 2020-08-25T08:21:34Z

src/tracer-config.ts

+  return { env, spec };
+}
+
+export async function getTracerConfig(


This function and tracerConfig have very similar names. Is it possible to rename them to make their different purposes more obvious? If not, please could you add a one-liner explaining what each one does?

I've changed to getTracerConfigForLanguage and getCombinedTracerConfig and I think that makes things a bit clearer.

sampart · 2020-08-25T08:22:27Z

src/tracer-config.ts

+  }
+
+  // Get all the tracer configs and combine them together
+  let tracedLanguageConfigs: { [lang: string]: TracerConfig } = {};


I think this can be a constants as you're not reassigning it, just updating its inner values.

Suggested change

let tracedLanguageConfigs: { [lang: string]: TracerConfig } = {};

const tracedLanguageConfigs: { [lang: string]: TracerConfig } = {};

robertbrignull · 2020-08-25T10:34:41Z

There are still a couple of places where the test names could be better, but I'm slightly at a loss of what to do. I'll merge this later on today and maybe we can improve things before then, but if not then the tests are something we can easily change after merging.

Break out tracer-config.ts

407ef0a

robertbrignull assigned sampart Aug 24, 2020

sampart approved these changes Aug 25, 2020

View reviewed changes

make tests clearer

82fb31e

Merge branch 'main' into tracer-config

8efabe9

robertbrignull force-pushed the tracer-config branch from 8eae9ab to 8efabe9 Compare August 25, 2020 10:35

Merge branch 'main' into tracer-config

2df51e0

robertbrignull merged commit c039c3b into main Aug 25, 2020

robertbrignull deleted the tracer-config branch August 25, 2020 14:20

github-actions bot mentioned this pull request Sep 1, 2020

Merge main into v1 #166

Merged

aliscco mentioned this pull request Apr 24, 2023

[Snyk] Fix for 10 vulnerabilities aliscco/codeql-action#67

Open

	// NB: in CLI mode these will be outputted to a file instead of exported with core.exportVariable
	// NB: in CLI mode these will be output to a file rather than exported with core.exportVariable

		// A very minimal setup
		test('tracerConfig - minimal', async t => {

	// A very minimal setup
	test('tracerConfig - minimal', async t => {
	test('tracerConfig - minimal setup', async t => {

	test('concatTracerConfigs - minimal', async t => {
	test('concatTracerConfigs - minimal configs correctly combined', async t => {

	t.deepEqual(undefined, await getTracerConfig(config, codeQL));
	t.deepEqual(await getTracerConfig(config, codeQL), undefined);

	let tracedLanguageConfigs: { [lang: string]: TracerConfig } = {};
	const tracedLanguageConfigs: { [lang: string]: TracerConfig } = {};

Break out tracer-config.ts to its own file #158

Break out tracer-config.ts to its own file #158

Uh oh!

Conversation

robertbrignull commented Aug 24, 2020

Merge / deployment checklist

Uh oh!

sampart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Aug 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants