Skip to content

Refactor arbitrary size values in SelectContentPage.spec.js tests for clarity #14782

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

Merged
marcellamaki merged 5 commits into from
Jun 4, 2026
+39 −35
Merged

Refactor arbitrary size values in SelectContentPage.spec.js tests for clarity #14782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e29c6fe
Refactor arbitrary size values in SelectContentPage.spec.js for clarity
akolson Jun 2, 2026
0bb4f42
Improve readability of remaining SelectContentPage test assertions
akolson Jun 2, 2026
9fe968a
Rename draftNewerVersion to newerVersion for clarity
akolson Jun 2, 2026
c31f35f
Address review feedback on PR #14782
akolson Jun 2, 2026
4f8e64c
Address second round of review feedback on PR #14782
akolson Jun 2, 2026
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
21 changes: 10 additions & 11 deletions kolibri/plugins/device/frontend/__tests__/utils/makeStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,16 @@ const allChannels = [
},
];

// The transferredChannel used by makeSelectContentPageStore, exported so tests can
// derive expected display values (resource counts, file sizes) from the same source of truth.
export const selectContentTransferredChannel = {
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot Jun 2, 2026

Choose a reason for hiding this comment

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

praise: Exporting selectContentTransferredChannel as a single source of truth is well-executed. It eliminates three previously-duplicated copies of on_device_resources: 2000 / on_device_file_size: 95189556 (in channelsOnDevice[0], in transferredChannel, and implicitly in the test expectations), and the spread of allChannels[0] means channel metadata like name, version, and description also flow through without duplication.

...allChannels[0],
on_device_resources: 2000,
on_device_file_size: 95189556, // about 95 MB
};

const channelsOnDevice = [
{
...allChannels[0],
on_device_resources: 2000,
on_device_file_size: 95189556, // about 95 MB
available: true,
},
{ ...selectContentTransferredChannel, available: true },
{
...allChannels[1],
on_device_resources: 0,
Expand Down Expand Up @@ -105,11 +108,7 @@ export function makeSelectContentPageStore() {
Object.assign(store.state.manageContent.wizard, {
availableChannels: [...allChannels],
transferType: 'localimport',
transferredChannel: {
...allChannels[0],
on_device_resources: 2000,
on_device_file_size: 95189556, // about 95 MB
},
transferredChannel: { ...selectContentTransferredChannel },
currentTopicNode: contentNodeGranularPayload(),
});
return store;
Expand Down
53 changes: 29 additions & 24 deletions kolibri/plugins/device/frontend/views/__tests__/SelectContentPage.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { render, screen } from '@testing-library/vue';
import { createTranslator, i18nSetup } from 'kolibri/utils/i18n';
import bytesForHumans from 'kolibri/uiText/bytesForHumans';
import SelectContentPage from '../SelectContentPage';
import { makeSelectContentPageStore } from '../../__tests__/utils/makeStore';
import {
makeSelectContentPageStore,
selectContentTransferredChannel,
} from '../../__tests__/utils/makeStore';
import ChannelContentsSummary from '../SelectContentPage/ChannelContentsSummary';
import ContentTreeViewer from '../SelectContentPage/ContentTreeViewer';
import NewChannelVersionBanner from '../ManageContentPage/NewChannelVersionBanner';
Expand Down Expand Up @@ -42,72 +46,75 @@ describe('SelectContentPage', () => {
});

it('shows the thumbnail, title, descripton, and version of the channel', () => {
const heading = 'Awesome Channel';
const version = '10';
const description = 'An awesome channel';
const { name, version, description } = selectContentTransferredChannel;
const fakeImage = 'data:image/png;base64,abcd1234';
updateMetaChannel(store, { thumbnail: fakeImage });
renderComponent({ store });
expect(screen.getByRole('img')).toHaveAttribute('src', fakeImage);
expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent(heading);
expect(screen.getByText(summaryTr.$tr('version', { version: version }))).toBeInTheDocument();
expect(screen.getByRole('heading', { level: 1 })).toHaveTextContent(name);
expect(screen.getByText(summaryTr.$tr('version', { version }))).toBeInTheDocument();
expect(screen.getByText(description)).toBeInTheDocument();
});

it('shows the total size of the channel', () => {
const { total_resources, total_file_size } = selectContentTransferredChannel;
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot Jun 2, 2026

Choose a reason for hiding this comment

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

praise: Extracting the fixture values before renderComponent now cleanly follows Arrange-Act-Assert — data setup is part of Arrange, not suspended mid-Act.

renderComponent({ store });
expect(screen.getAllByRole('row')[1]).toHaveTextContent(
`${summaryTr.$tr('totalSizeRow')} 1,000 5 GB`,
`${summaryTr.$tr('totalSizeRow')} ${total_resources.toLocaleString()} ${bytesForHumans(total_file_size)}`,
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot Jun 2, 2026

Choose a reason for hiding this comment

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

praise: Using bytesForHumans(total_file_size) and bytesForHumans(onDeviceFileSize) in assertions instead of hardcoded strings like "5 GB" / "95 MB" / "0 B" is the right approach. Tests now stay correct if the formatting utility ever changes its rounding or unit thresholds, and the intent — "what the component renders" — is made explicit through the same function the component uses.

);
});

it('shows the total size of any resources on the device', () => {
const { on_device_resources, on_device_file_size } = selectContentTransferredChannel;
renderComponent({ store });
expect(screen.getAllByRole('row')[2]).toHaveTextContent(
`${summaryTr.$tr('onDeviceRow')} 2,000 95 MB`,
`${summaryTr.$tr('onDeviceRow')} ${on_device_resources.toLocaleString()} ${bytesForHumans(on_device_file_size)}`,
);
});

it('shows size and resources as 0 if channel is not on device', () => {
const onDeviceResources = 0;
const onDeviceFileSize = 0;
updateMetaChannel(store, {
id: 'not_awesome_channel',
on_device_resources: 0,
on_device_file_size: 0,
on_device_resources: onDeviceResources,
on_device_file_size: onDeviceFileSize,
});
renderComponent({ store });
expect(screen.getAllByRole('row')[2]).toHaveTextContent(
`${summaryTr.$tr('onDeviceRow')} 0 0 B`,
`${summaryTr.$tr('onDeviceRow')} ${onDeviceResources.toLocaleString()} ${bytesForHumans(onDeviceFileSize)}`,
);
});

it('shows a update notification if a new version is available', () => {
updateMetaChannel(store, { version: 1000 });
const newVersion = 1000;
updateMetaChannel(store, { version: newVersion });
renderComponent({ store });
const NEW_VERSION = '1000';
expect(
screen.getByText(bannerTr.$tr('versionAvailable', { version: NEW_VERSION })),
screen.getByText(bannerTr.$tr('versionAvailable', { version: newVersion })),
).toBeInTheDocument();
});

it('if a new version is not available, then no notification/button appear', () => {
updateMetaChannel(store, { version: 10 }); // same version
const { version } = selectContentTransferredChannel;
updateMetaChannel(store, { version });
renderComponent({ store });
const NEW_VERSION = '1000';
expect(
screen.queryByText(bannerTr.$tr('versionAvailable', { version: NEW_VERSION })),
screen.queryByText(bannerTr.$tr('versionAvailable', { version })),
).not.toBeInTheDocument();
});

//Add translations strings to these tests & test these & commit the changes.
describe('draft channel (installed version = 0)', () => {
const newerVersion = 15;
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot Jun 2, 2026

Choose a reason for hiding this comment

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

praise: 15 is unambiguously greater than both the installed version (0) and the base channel version (10) — no more risk of a reader misreading "newer" as relative to the channel's base version field.

Copy link
Copy Markdown
Member

@marcellamaki marcellamaki Jun 4, 2026

Choose a reason for hiding this comment

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

15 is unambiguously greater than 0 and 10!! :)


function setInstalledVersion(store, version) {
const existing = store.state.manageContent.channelList[0];
store.commit('manageContent/SET_CHANNEL_LIST', [{ ...existing, version }]);
}

it('shows ContentTreeViewer when installed version is 0 and Studio has newer version', () => {
setInstalledVersion(store, 0);
updateMetaChannel(store, { version: 5 });
updateMetaChannel(store, { version: newerVersion });
renderComponent({ store });
expect(
screen.getByRole('checkbox', {
Expand All @@ -118,25 +125,23 @@ describe('SelectContentPage', () => {

it('shows NewChannelVersionBanner when installed version is 0 and Studio has newer version', () => {
setInstalledVersion(store, 0);
updateMetaChannel(store, { version: 5 });
updateMetaChannel(store, { version: newerVersion });
renderComponent({ store });
const NEW_VERSION = 5;
expect(
screen.getByText(bannerTr.$tr('versionAvailable', { version: NEW_VERSION })),
screen.getByText(bannerTr.$tr('versionAvailable', { version: newerVersion })),
).toBeInTheDocument();
});

it('shows SelectionBottomBar when installed version is 0 and Studio has newer version', () => {
setInstalledVersion(store, 0);
updateMetaChannel(store, { version: 5 });
updateMetaChannel(store, { version: newerVersion });
renderComponent({ store });
expect(
screen.getByRole('button', { name: bottomBarTr.$tr('importAction') }),
).toBeInTheDocument();
});

it('hides ContentTreeViewer when installed version > 0 and newer version available on Studio', () => {
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot Jun 2, 2026

Choose a reason for hiding this comment

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

nitpick: The comment // Preserve existing non-draft behavior was removed from line 144 (the hides ContentTreeViewer test). The code it described is still present and unchanged — the project conventions say to only remove comments when the described code is deleted or the comment is provably incorrect. The comment clarified why this non-draft test lives inside the draft channel describe block, which is genuinely useful context.

Copy link
Copy Markdown
Member Author

@akolson akolson Jun 2, 2026

Choose a reason for hiding this comment

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

The test's description is sufficient to provide all the context

Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot Jun 2, 2026

Choose a reason for hiding this comment

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

You're right — the test description provides the context readers need. Withdrawing this finding.

// Preserve existing non-draft behavior
updateMetaChannel(store, { version: 1000 });
renderComponent({ store });
expect(
Expand Down
Loading