-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Allow preset overrides #4601
Allow preset overrides #4601
Changes from all commits
4bac2ba
89454b0
795633b
26c69e0
de29c09
b1042e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import path from "node:path"; | ||
import util from "node:util"; | ||
import child from "node:child_process"; | ||
import {fileURLToPath} from "node:url"; | ||
import {expect, use} from "chai"; | ||
import chaiAsPromised from "chai-as-promised"; | ||
|
||
const scriptNames = { | ||
ok: "overridePresetOk.ts", | ||
error: "overridePresetError.ts", | ||
}; | ||
|
||
use(chaiAsPromised); | ||
|
||
const exec = util.promisify(child.exec); | ||
|
||
// Global variable __dirname no longer available in ES6 modules. | ||
// Solutions: https://stackoverflow.com/questions/46745014/alternative-for-dirname-in-node-js-when-using-es6-modules | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
const __dirname = path.dirname(fileURLToPath(import.meta.url)); | ||
const tsNodeBinary = path.join(__dirname, "../../../../node_modules/.bin/ts-node-esm"); | ||
|
||
describe("Override preset", function () { | ||
// Allow time for ts-node to compile Typescript source | ||
this.timeout(30_000); | ||
|
||
it("Should correctly override preset", async () => { | ||
await exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.ok)}`); | ||
Check warning Code scanning / CodeQL Shell command built from environment values
This shell command depends on an uncontrolled [absolute path](1).
This shell command depends on an uncontrolled [absolute path](2).
|
||
}); | ||
|
||
it("Should throw trying to override preset in the wrong order", async () => { | ||
await expect(exec(`${tsNodeBinary} ${path.join(__dirname, scriptNames.error)}`)).to.be.rejectedWith( | ||
Check warning Code scanning / CodeQL Shell command built from environment values
This shell command depends on an uncontrolled [absolute path](1).
This shell command depends on an uncontrolled [absolute path](2).
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you change this test to instead be split into two different files that does the same thing (one pass, one fail)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've tried to do it, but |
||
"Lodestar preset is already frozen" | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// This script is should be run in an e2e !! | ||
// It demostrates how NOT to change the Lodestar preset | ||
|
||
// 1. Import from not only @lodestar/params/setPreset will trigger an error | ||
import {SLOTS_PER_EPOCH} from "../../lib/index.js"; | ||
import {setActivePreset, PresetName} from "../../lib/setPreset.js"; | ||
// This line should throw | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
setActivePreset(PresetName.minimal, {SLOTS_PER_EPOCH: 2}); | ||
wemeetagain marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
SLOTS_PER_EPOCH; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import assert from "node:assert"; | ||
|
||
/* eslint import/order: "off" */ | ||
// This script is should be run in an e2e !! | ||
// It demostrates how to properly change the Lodestar preset safely | ||
|
||
// 1. Import from @lodestar/params/setPreset only | ||
import {setActivePreset, PresetName} from "../../src/setPreset.js"; | ||
// eslint-disable-next-line @typescript-eslint/naming-convention | ||
setActivePreset(PresetName.minimal, {SLOTS_PER_EPOCH: 2}); | ||
|
||
// 2. Import from any other @lodestar/params paths | ||
|
||
const {SLOTS_PER_EPOCH, BASE_REWARD_FACTOR} = await import("../../src/index.js"); | ||
|
||
assert.equal(SLOTS_PER_EPOCH, 2, "SLOTS_PER_EPOCH should have overriden preset value"); | ||
assert.equal(BASE_REWARD_FACTOR, 64, "BASE_REWARD_FACTOR should have preset value"); | ||
assert.equal(process.env.LODESTAR_PRESET, undefined, "LODESTAR_PRESET ENV must not be set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not extend setActivePreset() function above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. and make
overrides
optional.