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.

Sign up for GitHub

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: only skip ssz_static tests associated to missing type #6798

Merged
merged 3 commits into from
May 23, 2024
Merged

test: only skip ssz_static tests associated to missing type #6798

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36bff48
test: only skip ssz_static tests associated to missing type
nflaig May 16, 2024
1dee0f1
Merge branch 'electra-fork' into nflaig/ssz_static-tests
nflaig May 16, 2024
fe7e5ad
More detailed error message if type is not defined
nflaig May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 19 additions & 16 deletions packages/beacon-node/test/spec/presets/ssz_static.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,25 +57,28 @@ const sszStatic =
(ssz.altair as Types)[typeName] ||
(ssz.phase0 as Types)[typeName];

it(`SSZ type ${typeName} for fork ${fork} is defined`, function () {
expect(sszType).toBeDefined();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you specify an error message in expect()? Currently it throws AssertionError: expected undefined not to be undefined which doesn't make too much sense unless we look at the name of the test.

Something like

expect(sszType, `SSZ type ${typeName} for fork ${fork} is not defined`).toBeDefined()

and

it(`${fork} - ${typeName} existence check`)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you specify an error message in expect()?

This works but gives an eslint error

Expect takes most 1 argument eslint (vitest/valid-expect)

We have to use the custom function toEqualWithMessage in that case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssertionError: expected undefined not to be undefined which doesn't make too much sense unless we look at the name of the test.

Yes I totally agree with this but @nazarhussain has been pushing against having detailed assertion error messages so I kept it as is.

Right now if you wanna have a better assertion message you either have to

  • use toBeWithMessage / toEqualWithMessage which is crossed out as it is marked deprecated
  • or pass as 2nd param to expect and get an eslint error

We might wanna rethink this, imo it would be best if we override the vitest default toBe and toEqual to support the message as a 2nd param, and remove the custom toBeWithMessage and toEqualWithMessage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with @nazarhussain on this. The spec test name should be descriptive enough to explain what is being asserted, and there should ideally be only one expect in each test case so that its clear which assertion is failing. I tend to feel that adding more messaging to the test case means that the naming or assertion is incorrect or confusing and should be refactored

Copy link
Contributor

@ensi321 ensi321 May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image Maybe I am nitpicking or not used how vitest outputs logs, but when I debug I first look at the message in red, which in this case is not descriptive at all. I will then need to look at the unit test name, which is white and is prepended by file name and spec test name. image On the other hand, this is good because when I see the red message, I know `deposit_requests_root` is missing right from the first glance

});

if (!sszType) {
expect.fail(
`Missing SSZ type definition for ${typeName}; this will prevent associated ssz_static tests to be executed`
);
} else {
const sszTypeNoUint = replaceUintTypeWithUintBigintType(sszType);
// Return instead of throwing an error to only skip ssz_static tests associated to missing type
return;
}

const sszTypeNoUint = replaceUintTypeWithUintBigintType(sszType);

for (const testCase of fs.readdirSync(testSuiteDirpath)) {
// Do not manually skip tests here, do it in packages/beacon-node/test/spec/presets/index.test.ts
it(testCase, function () {
// Mainnet must deal with big full states and hash each one multiple times
if (ACTIVE_PRESET === "mainnet") {
vi.setConfig({testTimeout: 30 * 1000});
}
for (const testCase of fs.readdirSync(testSuiteDirpath)) {
// Do not manually skip tests here, do it in packages/beacon-node/test/spec/presets/index.test.ts
it(testCase, function () {
// Mainnet must deal with big full states and hash each one multiple times
if (ACTIVE_PRESET === "mainnet") {
vi.setConfig({testTimeout: 30 * 1000});
}

const testData = parseSszStaticTestcase(path.join(testSuiteDirpath, testCase));
runValidSszTest(sszTypeNoUint, testData);
});
}
const testData = parseSszStaticTestcase(path.join(testSuiteDirpath, testCase));
runValidSszTest(sszTypeNoUint, testData);
});
}
};

Expand Down
Loading