Adding serializer to remove internal angular attributes from the snapshots #97

sharikovvladislav · 2017-11-28T20:34:02Z

Hi!

I added serializer to remove the internal angular attributes from the result snapshots.

This is good because snapshots become more human readable. This is also good because sometimes angular renderer changes the indexes of that internal attributes.

The questions:

This change is HUGE BC change. So a lot of tests for components which are not simple as one line will be broken. do you mind bumping the major version of jest-preset-angular? It is also possible to add some flag which will enable this logic. So this change will not influence existing users.
I see you have CI. I think it should be improved a bit. Do you mind triggering your build on releasing new versions of Angular? If you do so, the time of understanding that something went wrong will be decreased significantly. What can go wrong? For example, Angular released a new version where they added some new property which is not covered by my serializer. It must be added. Also, this should be mentioned in the docs, I think...

sharikovvladislav · 2017-11-28T20:35:13Z

This is absolutely not final changes. It is just PR for showing the general idea of what I suggest. Let's say I created this for some discussion.

Technical stuff is really easy so we will fix it in a few moments.

sharikovvladislav · 2017-11-28T20:36:03Z

...src/app/a-bit-more-complex-component/__snapshots__/a-bit-more-complex.component.spec.ts.snap

+      <div
+    class="kek"
+  >
+


I don't like this formatting. Do you mind adding some format tool like prettier or stuff?

Do you know any formatting tool for jest snapshots? How is Jest doing this? They might have serializer for that but I can't find it.

They seem to use pretty-format with a custom React plugin

Hi @sharikovvladislav , @barretodavid,
There is a solution for this. Can you guys please check #86 discussion ?

...src/app/a-bit-more-complex-component/__snapshots__/a-bit-more-complex.component.spec.ts.snap

setupJest.js

sharikovvladislav · 2018-03-04T21:06:51Z

@thymikee any thoughts?

thymikee · 2018-03-04T22:08:42Z

I'd rather like this serializer to work on actual values instead of regexing things around. Is there a way you could add this functionality to the current one?

sharikovvladislav · 2018-03-04T22:12:28Z

What do you mean by "actual values"? Do you want me to modify existing serializer?

Now it is separated. Your serializer doing one thing. Another serializer extends yours by doing something extra. Do you think this approach is wrong?

thymikee · 2018-03-04T22:16:22Z

I think we should strive to have one that does the job well. By actual values I mean testing for objects with certain values as keys (e.g. _ngcontent). Also, you're running AngularSnapshotSerializer twice. It should suffice to do it just once.

sharikovvladislav · 2018-03-04T22:20:26Z

okay, as you wish!

what about general idea? what do you think about it? do you think it is good to have this feature in your tool?

if everything is ok I will spend a lot of time for this.

iMarv · 2018-05-16T09:55:19Z

If you create a dedicated describe-block for your snapshot tests, you can run enableProdMode() before building your test module. this will remove any debugging-directives (like ng-reflect-*) and probably result in the snapshot you are seeking

jfcere · 2018-09-10T17:38:55Z

Is this PR going to be merged some day?

I don't really know how and why but after updating to Angular 6 I found that a lot of _nghost and _ngcontent appeared in the snapshots ... I guess this PR would resolve the issue.

thymikee · 2018-09-18T16:03:32Z

Would love to in some form, but now it looks like the feedback wasn't addressed. Anybody willing to pick this up?

sharikovvladislav · 2018-09-24T14:16:08Z

Hey!

Well, question is still opened.

what about general idea? what do you think about it? do you think it is good to have this feature in your tool?

I am not sure you need this @thymikee , thats why I don't work on this PR.

thymikee · 2018-09-24T15:01:25Z

Not me, but the project would benefit from stripping some Angular internals.

sharikovvladislav · 2018-09-24T15:06:53Z

So do you mind merging upgraded version of this PR?

This is only one suggestion? #97 (comment) Anything else? I can fix this.

thymikee · 2018-09-24T15:20:37Z

My concern was #97 (comment) and actually invoking AngularSnapshotSerializer twice

sharikovvladislav · 2018-09-24T15:26:37Z

Ok, If you don't have any problems with general idea I will fix this 2 points and lets discuss it.

ghost · 2018-11-26T14:11:55Z

Hi! I'm following this thread since, it is getting too much hard for me to find a way to get rid of the ng-reflect- prefix in my template. I'm currently working on app route testing and need to do some directive filtering. The approach of adding enableProdMode() in a separate describe block (as @iMarv says here) makes DebugElement no to be available, hence I lose lot of features in order to make my tests work. Does it happen the same to you @iMarv ?
I've debugged official Angular examples for Route Testing and templates are compiled OK without ng-reflect- prefix. (Tests are developed with Karma and Jasmine). I don't see any call to enableProdMode() anywhere before running tests.
I tried to be as brief as possible. Sorry if I've made wrong assumptions. I'd be glad to give you more details in case you require them.
Thanks!

iMarv · 2018-11-26T15:13:45Z

@isidro1983 well the ng-reflect stuff should be part of "devMode", so enabling prod mode removes anything related to it.

We are sticking to the fully fledged snapshots now, as they can provide relevant information about what is given to inputs etc

listepo-alterpost · 2019-04-19T16:14:06Z

Any news?

sharikovvladislav · 2019-04-19T19:28:33Z

I will try to implement it this weekend. пт, 19 апр. 2019 г. в 19:14, listepo-alterpost <[email protected]>:

…

Any news? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#97 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABK4V7UJGJTH3Y5WEOL7WKLPRHVVBANCNFSM4EFXPQNA> .

sharikovvladislav · 2019-04-20T19:55:46Z

example/src/app/simple/__snapshots__/simple.component.spec.ts.snap

@@ -4,7 +4,6 @@ exports[`SimpleComponent snapshots 1`] = `
 <app-simple>
  <p>
     simple works!
-


Is it okay that I removed that empty lanes from existing snapshots? it is because of added filter function in serializer.

sharikovvladislav · 2019-04-20T19:57:06Z

Any news?

Done.

@thymikee what do you think?

sharikovvladislav · 2019-04-20T19:59:14Z

In general:

I just added few lines of code into existing angular snapshot serializer.
I also added a bit complex component example.

@thymikee
Notice, that this is huge BC and you should bump major version to safely publish this code feature.

Probably, you want to add this behind some configuration flag? What do you think?

Also, probably somebody want to see debug attributes in snapshots. For example from message above:

We are sticking to the fully fledged snapshots now, as they can provide relevant information about what is given to inputs etc

So may be it is better to apply this feature behind some config flag.

ahnpnl · 2019-04-20T20:00:43Z

Angular v8 is almost there, I guess we can merge and hold for release once Angular v8 releases then we can release after that.

wtho

What is still missing in my opinion is a test where a kek-attribute-content would collide with the angular naming pattern, like kek="_ngcontent", but it will still be rendered, as it is not the attribute name conflicting with the patterns.

Also pure text content like <span>ng-reflect-whatever</span> could be included to test edge cases.

We should also give the users a fallback-possibility to use the old serializer in case they still want to keep the old serializing behavior. So this PR could be implemented in a second serializer. If it is worth it, the two serializers can share common code.

As long as we do not change the current serializer behavior, there is no need for a breaking change. The new serializers could be made default at a later point in time.

Another thing that does not seem to be discussed here is class-names in components. They get indexed and each time the test runs in a different order, the indeces are different, e. g. class="ng-tns-c1-1".

Angular also adds attributes to inner non-component html elments like through directives and animations. I think these are not handled by this serializer, but I might be wrong.

wtho · 2019-04-20T21:05:44Z

example/src/app/a-bit-more-complex-component/a-bit-more-complex.component.spec.ts

+  it('snapshot test', () => {
+    expect(fixture).toMatchSnapshot();
+  });
+});


The component, test file and the test suite should not just be named "a bit more complex component", we have also other tests than the serializers, so it should definitely explain, that it is a serializer-test.

You can name it something like "complex templating component" or name what is complex about it.

Actually, this code was wrote very long time ago, but still... It was "copy-pasted" from other specs in example project.

example/src/app/calc/calc.component.spec.ts:

it('should snap', () => { expect(fixture).toMatchSnapshot(); });

example/src/app/simple/simple.component.spec.ts:

it('snapshots', () => { expect(fixture).toMatchSnapshot(); })

And stuff...

I understand your point. Is it critical? If yes, other code have to be refactored too.

I see. Still I think "SimpleComponent" alone is clear what to expect in the component. "ABitMoreComplexComponent" will always only stand in comparison to "SimpleComponent", which is not as good.

I would recommend e. g. "MediumComplexComponent"

wtho · 2019-04-20T21:09:51Z

AngularSnapshotSerializer.js

@@ -29,6 +29,17 @@ const print = (val, print, indent, opts, colors) => {
    .map(node => Array.from(node.renderElement.childNodes).map(print).join(''))
    .join(opts.edgeSpacing);

+
+  const nodesWithoutDebugInformation = nodes
+    .split('\n')


What if opts.edgeSpacing (see line 30) is not set to \n? I think this might not work anymore.

Better make your transformations before the html gets joined.

Probably, I can just split and join lines by opts.edgeSpacing? What do you think?

In line 30 the join is applied. If you use the nodes without joining them there you do not have to split them.

Your join afterwards should use opts.edgeSpacing then.

sharikovvladislav · 2019-04-21T07:11:57Z

@wtho very good points. And actually my code does not handle such cases it all. Such text will be removed.

Angular also adds attributes to inner non-component html elments like through directives and animations. I think these are not handled by this serializer, but I might be wrong.

Can you give some examples? Especialy animations.

TODO:

do not remove _ngcontent/_nghost/ng-reflect-* text if it is passed as real HTML, but was not generated by Angular and add units for such cases
we need some configuration flag about new behavior of serializer
check and handle class="ng-tns-c1-1 cases

sharikovvladislav · 2019-04-21T08:44:41Z

Another thing that does not seem to be discussed here is class-names in components. They get indexed and each time the test runs in a different order, the indeces are different, e. g. class="ng-tns-c1-1".

Can you give some example of such behavior?

I tried to get this. Here is the example: click me. I can not see any indexes in class list.

This is what I get

wtho · 2019-04-21T10:14:23Z

I build a small example with a directive and an animation, but luckily both do not add any attributes to the html, they both just add more classes.

https://stackblitz.com/edit/angular-hqvsrr?file=src%2Fapp%2Fapp.component.html

<p _ngcontent-xqo-c12 class="ng-tns-c12-0 test" appsomedirective>
  directive
</p>
<p _ngcontent-xqo-c12 class="ng-tns-c12-0 ng-trigger ng-trigger-triggerName" style>
  animation
</p>
<p _ngcontent-xqo-c12>
  text
</p>

Still all children get the same _ngcontent-attribute, which should also be handled in some way.

The current serializer does not render non-angular component html, it lets pretty-format (included in jest) do the job. I think a good solution in the long run would be to use pretty-format, but tell it to handle specific attributes different.
Sadly there is no API in pretty-format to do that, but we can rewrite the existing pretty-format-plugin DOMElement.js and call pretty-format with the plugin to obtain cleaned up html.

This is just a suggestion and open for discussion.

EDIT:

I can not see any indexes in class list.

Yeah I remember they are added through the animations module. As soon as you add BrowserAnimationsModule to some module, the elements inside this module will get the indexed classes. See my example.

sharikovvladislav · 2019-04-21T11:20:26Z

the elements inside this module will get the indexed classes. See my example.

So do you suggest to remove such technical (for animations, like ng-tns-c6-0) classes in jest-preset-angular serializers? What should I do in this PR?

The current serializer does not render non-angular component html

Sorry, I didn't get. How this paragraph relates to my PR? What exactly you want me to do in this PR?

we need some configuration flag about new behavior of serializer

Also, lets discuss this option. This have to be done in this PR. We don't want breaking changes after implementing this feature, do we? So this feature has to be hidden behind some configuration flag. By default, this feature will be disabled. If people like this feature, default value of that configuration may be changed in some major version in the future.
Am I right? So I will add some config flag with false default value, so this feature would not be applied to existing snapshots in existing projects, which are using jest-preset-angular. Also, docs should be updated.

UPD:
Opinion about formatting snapshots HTML. Seems to me, jest applies prettier formatter by default (if prettier is installed to your project). Looks like, this feature is ignored in jest-preset-angular snapshotting mechanisms. Even if you have prettier installed, snapshots will ignore it and would not be formatted.

prettier-format is about this?

I thought we could just optionally import prettier (according to this option https://jestjs.io/docs/en/configuration#prettierpath-string) and if it exists, just pass generated html (by lib serializers) throught it. Am I missing something?

sharikovvladislav · 2019-04-27T11:36:30Z

@sharikovvladislav one more thing, I really would prefer a-bit-more-complex.component renamed to complex.component or medium-complex.component, to not be relative to another component, but have a valid standalone name.

If you do not like this naming change, I am ok with it, but I just did not want to leave this undiscussed.

Done.

ahnpnl · 2019-04-27T19:25:31Z

i think all good to go

wtho · 2019-04-27T21:03:03Z

@thymikee could you please have a look?

By putting it as an optional serializer for now we can immediately publish a new minor version and do not have to hold it back.
Later on release of v8 we can use it as the main serializer, as several people preferred the cleaner html without angular debug properties.
Opting out will still be easy, as it does not touch the old serializer.

Also this PR fixes some issue with CI that we did not stumble upon before: we cache example/node_modules to speed up CI, but this leads to jest-preset-angular in example/node_modules to be cached and reused, always the same version whichever is currently in the cache (which is no issue for yarn as long as the version in package.json is the same).

So changes in the root folder were not respected by the example app, as it used a previously cached version of jest-preset-angular. This made the tests fail.

This is now fixed by not using cache in the example app. Using the cache and then just reinstalling jest-preset-angular somehow took way more time than just running the complete dependency installation.

For me everything looks good in this PR 👍
Thanks again @sharikovvladislav for all the effort!

thymikee · 2019-04-27T22:13:42Z

Gonna have a look at this tomorrow. I'm so impressed by the amount of work you all put into this!

thymikee

Looks way better now. Left some comments to address :)

thymikee · 2019-04-28T07:31:45Z

README.md

@@ -1,4 +1,4 @@
-# jest-preset-angular
+AngularSnapshotSerializer# jest-preset-angular


please revert

Oops. Done.

thymikee · 2019-04-28T07:32:17Z

README.md

@@ -69,6 +69,7 @@ module.exports = {
  },
  transformIgnorePatterns: ['node_modules/(?!@ngrx)'],
  snapshotSerializers: [
+    'jest-preset-angular/AngularNoNgAttributesSnapshotSerializer.js',


it's not included by default yet, please revert

thymikee · 2019-04-28T07:32:53Z

example/package.json

@@ -8,7 +8,7 @@
    "build": "ng build",
    "test": "jest",
    "test:watch": "jest --watch",
-    "test:ci": "jest --runInBand",
+    "test:ci": "jest --runInBand --no-cache",


why no-cache?

@wtho looks like it was your change... do we still need it?

It can be removed!

thymikee · 2019-04-28T07:33:50Z

example/src/app/a-bit-more-complex-component/a-bit-more-complex.component.spec.ts

@@ -0,0 +1,34 @@
+/* tslint:disable:no-unused-variable */


future notice: we should move to eslint with TS support

Yeah!
But I would suggest to wait for Angular to make the official move to eslint, as this is still an Angular app example (might come in v8?).

Also see the official tslint -> eslint issue

example/src/app/a-bit-more-complex-component/a-bit-more-complex.component.ts

example/src/app/simple-with-styles/simple-with-styles.component.ts

package.json

AngularNoNgAttributesSnapshotSerializer.js

thymikee · 2019-04-28T08:05:52Z

Also, please make sure CI is green (looks like something is leaking)

wtho · 2019-04-28T13:03:05Z

I'll fix the CI later 👍

wtho · 2019-04-28T19:55:18Z

How can I find out about leaks in the test runs?

I ran the tests and they exceeded 10 minutes without output on CircleCI.

Then I added the --detectOpenHandles flag and the tests ran successfully.

The only change between the test runs was:

- yarn test:ci
+ yarn test:ci --detectOpenHandles
- yarn test:coverage
+ yarn test:coverage --detectOpenHandles

Unfortunately there is no information about possible leaks in the output.

thymikee · 2019-04-28T19:59:47Z

README.md

@@ -84,7 +84,8 @@ module.exports = {
 - `"moduleFileExtensions"` – our modules are TypeScript and JavaScript files
 - `"moduleNameMapper"` – if you're using absolute imports here's how to tell Jest where to look for them; uses regex
 - `"setupFilesAfterEnv"` – this is the heart of our config, in this file we'll setup and patch environment within tests are running
- `"transformIgnorePatterns"` – unfortunately some modules (like @ngrx ) are released as TypeScript files, not pure JavaScript; in such cases we cannot ignore them (all node_modules are ignored by default), so they can be transformed through TS compiler like any other module in our project.
+- `"transformIgnorePatterns"` – unfortunately some modules (like @ngrx) are released as TypeScript files, not pure JavaScript; in such cases we cannot ignore them (all node_modules are ignored by default), so they can be transformed through TS compiler like any other module in our project.
+- `"snapshotSerializers"` - array of serializers which will be applied to snapshot the code. Note: by default angular adds some angular-specific attributes to the code (like `ng-reflect-*`, `ng-version="*"`, `_ngcontent-c*` etc). This package provides serializer to remove such attributes. This makes snapshots cleaner and more human-readable. To remove such specific attributes use `AngularNoNgAttributesSnapshotSerializer` serializer. You need to add `AngularNoNgAttributesSnapshotSerializer` serializer manually (like in `example` application in `package.json`).


can we add a link to the exact lines in package.json?

thymikee · 2019-04-28T20:02:17Z

Feel free to merge it @wtho :)

ahnpnl · 2019-04-29T05:25:53Z

Looking forward to test this in real projects 👍

sharikovvladislav · 2019-04-29T11:40:47Z

@listepo-alterpost @barretodavid @isoden @iam4x @Lykathia @jfcere

Any feedback?

Lykathia · 2019-04-29T13:24:20Z

I won't be able to check for a couple weeks now, unfortunately. Looking forward to it tho!

sharikovvladislav commented Nov 28, 2017

View reviewed changes

ahnpnl reviewed Dec 17, 2017

View reviewed changes

setupJest.js Outdated Show resolved Hide resolved

sharikovvladislav force-pushed the master branch from b342224 to ba496a4 Compare April 20, 2019 19:48

sharikovvladislav commented Apr 20, 2019

View reviewed changes

wtho reviewed Apr 20, 2019

View reviewed changes

wtho force-pushed the master branch 5 times, most recently from 03e6786 to 33b1827 Compare April 27, 2019 20:58

wtho requested a review from thymikee April 27, 2019 21:03

thymikee requested changes Apr 28, 2019

View reviewed changes

Vladislav.Sharikov and others added 2 commits April 28, 2019 12:32

refactor(*): rename component

d8577b9

chore(ci): fully disable cache to ensure currency

d656546

sharikovvladislav force-pushed the master branch from 33b1827 to 1617874 Compare April 28, 2019 09:42

sharikovvladislav force-pushed the master branch from 1617874 to bf05f6f Compare April 28, 2019 17:39

refactor(*): some points

a582354

sharikovvladislav force-pushed the master branch from bf05f6f to a582354 Compare April 28, 2019 17:44

thymikee reviewed Apr 28, 2019

View reviewed changes

thymikee approved these changes Apr 28, 2019

View reviewed changes

remove --no-cache flag, add serializer config link

61ad6a4

wtho merged commit 1079528 into thymikee:master Apr 28, 2019

ahnpnl mentioned this pull request May 17, 2019

Different snapshot with --coverage #220

Closed

ravishivt mentioned this pull request Dec 21, 2019

Remove more internal angular attributes from snapshots #336

Closed

		@@ -1,4 +1,4 @@
		# jest-preset-angular
		AngularSnapshotSerializer# jest-preset-angular

Adding serializer to remove internal angular attributes from the snapshots #97

Adding serializer to remove internal angular attributes from the snapshots #97

Conversation

sharikovvladislav commented Nov 28, 2017

sharikovvladislav commented Nov 28, 2017

sharikovvladislav Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharikovvladislav commented Mar 4, 2018

thymikee commented Mar 4, 2018

sharikovvladislav commented Mar 4, 2018

thymikee commented Mar 4, 2018

sharikovvladislav commented Mar 4, 2018 • edited Loading

iMarv commented May 16, 2018

jfcere commented Sep 10, 2018

thymikee commented Sep 18, 2018

sharikovvladislav commented Sep 24, 2018

thymikee commented Sep 24, 2018

sharikovvladislav commented Sep 24, 2018

thymikee commented Sep 24, 2018

sharikovvladislav commented Sep 24, 2018

ghost commented Nov 26, 2018

iMarv commented Nov 26, 2018

listepo-alterpost commented Apr 19, 2019

sharikovvladislav commented Apr 19, 2019 via email

Choose a reason for hiding this comment

sharikovvladislav commented Apr 20, 2019 • edited Loading

sharikovvladislav commented Apr 20, 2019 • edited Loading

ahnpnl commented Apr 20, 2019 • edited Loading

wtho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharikovvladislav commented Apr 21, 2019 • edited Loading

sharikovvladislav commented Apr 21, 2019 • edited Loading

wtho commented Apr 21, 2019 • edited Loading

sharikovvladislav commented Apr 21, 2019 • edited Loading

sharikovvladislav commented Apr 27, 2019

ahnpnl commented Apr 27, 2019

wtho commented Apr 27, 2019

thymikee commented Apr 27, 2019 • edited Loading

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee commented Apr 28, 2019

wtho commented Apr 28, 2019

wtho commented Apr 28, 2019

Choose a reason for hiding this comment

thymikee commented Apr 28, 2019

ahnpnl commented Apr 29, 2019

sharikovvladislav commented Apr 29, 2019 • edited Loading

Lykathia commented Apr 29, 2019

sharikovvladislav Nov 28, 2017 •

edited

Loading

sharikovvladislav commented Mar 4, 2018 •

edited

Loading

sharikovvladislav commented Apr 20, 2019 •

edited

Loading

sharikovvladislav commented Apr 20, 2019 •

edited

Loading

ahnpnl commented Apr 20, 2019 •

edited

Loading

sharikovvladislav commented Apr 21, 2019 •

edited

Loading

sharikovvladislav commented Apr 21, 2019 •

edited

Loading

wtho commented Apr 21, 2019 •

edited

Loading

sharikovvladislav commented Apr 21, 2019 •

edited

Loading

thymikee commented Apr 27, 2019 •

edited

Loading

sharikovvladislav commented Apr 29, 2019 •

edited

Loading