Skip to content
This repository was archived by the owner on Jan 14, 2024. It is now read-only.

isolate tests in exercise#260

Open
shieldo wants to merge 2 commits into
base:main
Choose a base branch
from
Open

isolate tests in exercise #260

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8ac96d
isolate tests in exercise
shieldo Mar 9, 2023
0a68d5f
add expectations that arrays have not changed value in ft exercise
shieldo Mar 11, 2023
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
48 changes: 32 additions & 16 deletions 2-mandatory/2-financial-times.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -37,45 +37,61 @@ function averageNumberOfCharacters(allArticleTitles) {


/* ======= List of Articles - DO NOT MODIFY ===== */
const ARTICLE_TITLES = [
"Streaming wars drive media groups to spend more than $100bn on new content",
"Amazon Prime Video India country head: streaming is driving a TV revolution",
"Aerospace chiefs prepare for bumpy ride in recovery of long-haul flights",
"British companies look to muscle in on US retail investing boom",
"Libor to take firm step towards oblivion on New Year's Day",
"Audit profession unattractive to new recruits, says PwC boss",
"Chinese social media users blast Elon Musk over near miss in space",
"Companies raise over $12tn in 'blockbuster' year for global capital markets",
"The three questions that dominate investment",
"Brussels urges Chile's incoming president to endorse EU trade deal",
];

@sztupysztupyMar 9, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What about doing instead

Object.freeze(ARTICLE_TITLES);

This would cause the trainee receiving a object is not extensible exception in case they accidentally try to push values into this constant.

(note I come from ruby where it is usually expected to explicitly freeze constants so they become inmutable, not sure if this is as well known in the JS world or not)

@shieldoshieldoMar 9, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for this feedback!

I think I'm against this for two reasons:

  1. I'm making the argument here to make mutability not a problem at this stage (so as to not overload the exercise) - tests like this should be isolated because the order in which they execute is non-deterministic, so this situation has arisen out of bad practice. It's reasonable that mutations are commented on I think in code review, but not reasonable that they make code fail with an error that assumes a lot of knowledge for this stage

  2. I'd argue that tests should always be reasonably legible to trainees at whatever level of the course they are at, and referencing the Object class I'd argue is premature. If trainees were to look up Object.freeze and understand what it did, I'm not sure that the reason for using it here would be clear.

I wasn't aware of Object.freeze in JavaScript, actually, and am used to calling .freeze on objects in Ruby that are constants! The equivalent thing to do here I think would be to freeze the array on assigning it to ARTICLE_TITLES, but I'm not convinced it's the right approach.

@sztupysztupyMar 10, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe we should teach the trainees that mutability is usually not accepted, even if not explicitly stated on the exercise level at least in the tests - the tests should explicitly check that the array that was sent in remained unchanged. If any trainees gets blocked by this they could always ask. On the other hand if they send in a solution which does change the input and no one tells them that's usually not acepatable they could get lost later. I actually did receive a couple questions related on this on why they needed to create a new array in these solutions, although they could just change the parameter in one of the week2 solutions (https://github.com/CodeYourFuture/JavaScript-Core-1-Coursework-Week2-Solution/blob/main/mandatory/2-function-creation.js#L81 ) which was a good question on multiple fronts (immutability, variable assignments, difference between changing an object and assigning a new value to a variable, etc)

On point 2, that could always be solved with a simple comment, like // make sure that the list of article can not be accidentally changed. But as you mentioned freeze is not as known in JS land, so I'm happy to swap it's usage to explicit checks that the functions created are pure and don't have side effects

function articleTitles() {
return [
"Streaming wars drive media groups to spend more than $100bn on new content",
"Amazon Prime Video India country head: streaming is driving a TV revolution",
"Aerospace chiefs prepare for bumpy ride in recovery of long-haul flights",
"British companies look to muscle in on US retail investing boom",
"Libor to take firm step towards oblivion on New Year's Day",
"Audit profession unattractive to new recruits, says PwC boss",
"Chinese social media users blast Elon Musk over near miss in space",
"Companies raise over $12tn in 'blockbuster' year for global capital markets",
"The three questions that dominate investment",
"Brussels urges Chile's incoming president to endorse EU trade deal",
];
}

/* ======= TESTS - DO NOT MODIFY ===== */

test("should only return potential headlines", () => {
expect(new Set(potentialHeadlines(ARTICLE_TITLES))).toEqual(new Set([
const titles = articleTitles();
expect(new Set(potentialHeadlines(titles))).toEqual(new Set([
"British companies look to muscle in on US retail investing boom",
"Libor to take firm step towards oblivion on New Year's Day",
"Audit profession unattractive to new recruits, says PwC boss",
"The three questions that dominate investment"
]));
expectArrayIsUnchanged(titles, articleTitles());
});

test("should return an empty array for empty input", () => {
expect(potentialHeadlines([])).toEqual([]);
});

test("should return the title with the fewest words", () => {
expect(titleWithFewestWords(ARTICLE_TITLES)).toEqual("The three questions that dominate investment");
const titles = articleTitles();
expect(titleWithFewestWords(articleTitles())).toEqual("The three questions that dominate investment");
expectArrayIsUnchanged(titles, articleTitles());
});

test("should only return headlines containing numbers", () => {
expect(new Set(headlinesWithNumbers(ARTICLE_TITLES))).toEqual(new Set([
const titles = articleTitles();
expect(new Set(headlinesWithNumbers(titles))).toEqual(new Set([
"Streaming wars drive media groups to spend more than $100bn on new content",
"Companies raise over $12tn in 'blockbuster' year for global capital markets"
]));
expectArrayIsUnchanged(titles, articleTitles());
});

test("should return the average number of characters in a headline", () => {
expect(averageNumberOfCharacters(ARTICLE_TITLES)).toEqual(65);
const titles = articleTitles();
expect(averageNumberOfCharacters(titles)).toEqual(65);
expectArrayIsUnchanged(titles, articleTitles());
});

function expectArrayIsUnchanged(first, second) {
// arguments to functions should not be modified
// this expectation checks arrays have the same values, to make sure arguments have not been changed
expect(first).toEqual(second);
}