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

Mews Assignment #1

Open
wants to merge 56 commits into
base: initial-commit
Choose a base branch
from
Open

Mews Assignment #1

wants to merge 56 commits into from

Conversation

RonanCodes
Copy link
Owner

No description provided.

Copy link

nx-cloud bot commented May 7, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 987b91e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 3 targets

Sent with 💌 from NxCloud.

Copy link

@karelzibar karelzibar left a comment

Choose a reason for hiding this comment

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

Hi Ronan.
I comment here just some basic questions so we can have discusion later. For now have a nice day and Paula will arange a next round of interview with you ;)


public searchMovies(searchQuery?: string, page = 1): void {
if (searchQuery) {
this.searchQuery = searchQuery;

Choose a reason for hiding this comment

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

is it for future use or ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is so that when we go into the movie detail page, then back to the search page, and click next page, that the pagination continues to work.

If the search query was not saved then the pagination would not work.

});

it('should be created', () => {
expect(service).toBeTruthy();

Choose a reason for hiding this comment

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

this test tests something that happens outside of it ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is the default test that comes with service.
It just checks that the service has instantiated correctly.

});

it('should be created', () => {
expect(service).toBeTruthy();

Choose a reason for hiding this comment

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

this test tests something that happens outside of it ?

Choose a reason for hiding this comment

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

also nice that you wrote tests. But they do nothing usefull :D

Copy link
Owner Author

Choose a reason for hiding this comment

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

So this is the default test that comes with the service :)
It just checks that the service has instantiated correctly.


it('should render title', () => {
const fixture = TestBed.createComponent(HomeComponent);
fixture.detectChanges();

Choose a reason for hiding this comment

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

why not use global fixture and component that is handled in beforeEach ?

Copy link
Owner Author

@RonanCodes RonanCodes May 15, 2024

Choose a reason for hiding this comment

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

For this piece of code I actually should have remove line 33 since I already have the fixture defined on line 23.

This was a miss on my side.
Line 33 was the default code that I forgot to remove.

fixture.detectChanges();
const compiled = fixture.nativeElement as HTMLElement;
expect(
compiled.querySelector('mat-toolbar span.title')?.textContent

Choose a reason for hiding this comment

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

what can we do to prevent failure of this selector when DOM is changed ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The selector mat-toolbar span.title will always be present in this component so it should never fail.

Could you elaborate on the potential failure?

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