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

Fix JSONSerializer to pass through payload.included #3550

Merged
merged 1 commit into from
Jul 16, 2015

Conversation

tstirrat
Copy link
Contributor

Respect an underlying JSON-API payload's included property.

Prior to this, records extracted via EmbeddedRecordsMixin were being truncated and forgotten.

See #3549

@tstirrat
Copy link
Contributor Author

The tests pass locally for me but I am sure my test is not actually running!!

is there some way to isolate just this test? or do integration tests need separate command?

@tstirrat tstirrat force-pushed the ts-jsonapi-keep-included branch from b6c931f to 3a2a0d9 Compare July 15, 2015 19:36
@fivetanley
Copy link
Member

If you use npm start, then visit on localhost:4200, you can click on the individual test, or use the dropdown menu, to isolate tests or suites of tests.

@tstirrat
Copy link
Contributor Author

@fivetanley ok thanks, got it!

My tests are not running with npm test. is this a problem?

@fivetanley
Copy link
Member

It looks like it is, at least on travis: https://travis-ci.org/emberjs/data/builds/71138302#L552

@tstirrat
Copy link
Contributor Author

They run with npm start and they run on travis.. but not locally.

as a test, I deleted the entire /packages/ember-data/tests/unit folder and it made no difference to the total test count.

975 tests complete (11519 ms)

surely that should have dropped by at least 100..

Anyway, I can get by using npm start but this shows something is weird with npm test

@fivetanley
Copy link
Member

npm test doesn't do a full build, so you're likely getting an old build. we should probably fix that.

to re-run with npm test, use ember build --environment production and then npm test.

@tstirrat tstirrat force-pushed the ts-jsonapi-keep-included branch from 3a2a0d9 to 672e47e Compare July 15, 2015 20:57
@tstirrat
Copy link
Contributor Author

ok, np, tests are passing now.

@kmiyashiro
Copy link
Contributor

Would love for embedded records to not be broken 👍

documentHash.data = data;
documentHash.included = included;
Copy link
Contributor

Choose a reason for hiding this comment

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

included could be undefined here, we should add a check

@tstirrat tstirrat force-pushed the ts-jsonapi-keep-included branch from 672e47e to ab614cc Compare July 16, 2015 14:18
@tstirrat
Copy link
Contributor Author

@wecc fixed, added isolating test

wecc added a commit that referenced this pull request Jul 16, 2015
Fix JSONSerializer to pass through `payload.included`
@wecc wecc merged commit 09556fc into emberjs:master Jul 16, 2015
@wecc
Copy link
Contributor

wecc commented Jul 16, 2015

Awesome, thanks @tstirrat!

ping @bmac, release

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.

4 participants