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.

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

feat: add testing for plo endpoints #512

Merged
merged 27 commits into from
Apr 12, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Apr 9, 2021

Added:

  • Rococo Registry and metadata
  • TypeFactory class
  • Tests for the following methods
    • ParasService.crowdloansInfo
    • ParasService.crowdloans
    • ParasService.leaseInfo
    • ParasService.leasesCurrent
    • ParasService.paras
    • ParasService.auctionsCurrent

@TarikGul TarikGul requested a review from emostov April 9, 2021 05:47
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

Overall it looks good, but some minor grumbles

src/services/paras/ParasService.spec.ts Outdated Show resolved Hide resolved
src/services/paras/ParasService.spec.ts Outdated Show resolved Hide resolved
src/services/paras/ParasService.spec.ts Outdated Show resolved Hide resolved
src/services/paras/ParasService.spec.ts Outdated Show resolved Hide resolved
src/services/paras/ParasService.spec.ts Outdated Show resolved Hide resolved
*/
const beingEnd = new BN(1000) as AbstractInt;
const leasePeriodIndex = new BN(39) as AbstractInt;
const vectorAuctions = typeFactory.vecOf([beingEnd, leasePeriodIndex]);
Copy link
Contributor

Choose a reason for hiding this comment

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

auctions::AuctionInfo is a Option<(LeasePeriodOf<T>, T::BlockNumber)> so Option<[beginEnd, leasePeriodIndex]>. I don't see the Option part here. In the service we have it modelled as Option<Vec<AbstractInt>> - should wrap this in an Option

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea so this is actually wrapped in an Option inside of auctionInfoAt, but there were two methods using vectorAuction's so I didn't wrap it in an Option in the outer scope. But you raised a good point with a previous comment typeFactory.optionOf(null) so I fixed it all to be more on point, and now the variable are all inside of auctionInfoAt and don't need to be in the outer scope anymore.

Inside of TypeFactory we now have a nullOf, and that allows you too create a Null primitive type and pass it into Option. Good catch and great addition.

parasOptionsOne,
parasOptionsTwo,
]);
const optionWinnings = typeFactory.optionOf(vectorWinnings);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be longer? like a length of 7 (Or whatever the SLOT_RANGE_COUNT is)? The rest of the values can just be Option::None, but I am concerned there could be behavior that relies on the length of auctions::Winning.

It is defined as [Option<(<T as frame_system::Config>::AccountId, ParaId, BalanceOf<T>)>; SlotRange::SLOT_RANGE_COUNT] where SLOT_RANGE_COUNT is the length it will always be.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I couldnt find where it says the minimum SLOT_RANGE_COUNT is, but I know it's suppose to be a maximum of 10 per the older code about a week ago.

/// Total number of possible sub ranges of slots.
pub const SLOT_RANGE_COUNT: usize = 10;

This was the comment that they had for slot range.

Now the codes changed a bit which includes a generate_slot_range macro.

/// * A constant SLOT_RANGE_COUNT will count the total number of enum variants.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets hardcode it to 10 (I think we covered the rest in offline discussion.)

src/test-helpers/registries/rococoRegistry.ts Outdated Show resolved Hide resolved
src/test-helpers/typeFactory.ts Show resolved Hide resolved
src/test-helpers/typeFactory.ts Show resolved Hide resolved
@TarikGul
Copy link
Member Author

@emostov I restructured the tests so that they all reflect 'user contract' tests. They all use sanitizeNumbers as well as toMatchObject to mock an actual user response. I also added a typeFactory.nullOf for Null primitives that can be added to Option. Everything is now looking good, the only last piece is the Slot range constant that is a little confusing. Whats I guess a little confusing for me is the macro itself generate_slot_range!.

Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

LGTM % nits

src/services/paras/ParasService.spec.ts Outdated Show resolved Hide resolved
expect(sanitizeNumbers(response)).toMatchObject(expectedResponse);
});

it('Should return the correct null values when auctionInfo is not of type Option', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('Should return the correct null values when auctionInfo is not of type Option', async () => {
it('Should return the correct null values when auctionInfo is `None`', async () => {

It should be of type Option: Option::None. In JSON though that gets converted into null.

Copy link
Contributor

Choose a reason for hiding this comment

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

The sanitizeNumber specs are exhaustive for this stuff. See

it('converts None to null', () => {
const none = kusamaRegistry.createType('Option<Text>', null);
expect(sanitizeNumbers(none)).toBe(null);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh gotcha, yea that makes sense. Empty Options are returned as null with sanitize numbers :)!

src/services/paras/ParasService.spec.ts Show resolved Hide resolved
export const nullAuctionsInfoAt = (): Promise<Option<Null>> =>
Promise.resolve().then(() => {
const nullPrimitive = typeFactory.nullOf();
const nullOption = typeFactory.optionOf(nullPrimitive);
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 just pass in null here instead of creating this more complicated polkadot-js null thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well no is the simplest answer. The way its implemented to in polkadot/api is to create a emptyOption method. Which is what I just added. It's definitely better than adding the nullOf method as you mentioned.

public emptyOption = <T extends Codec>(typeName: keyof InterfaceTypes): Option<T> => 
        new Option<T>(this.#registry, typeName);

And then we just add the Null primitive as the typeName

parasOptionsOne,
parasOptionsTwo,
]);
const optionWinnings = typeFactory.optionOf(vectorWinnings);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets hardcode it to 10 (I think we covered the rest in offline discussion.)

src/test-helpers/typeFactory.ts Outdated Show resolved Hide resolved
@TarikGul TarikGul merged commit 5eff845 into feat-experiment-plo-endpoints Apr 12, 2021
@TarikGul TarikGul deleted the tarik-plo-tests branch April 12, 2021 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants