Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

Duplicate CRUD Test - Profile Picture #1028

Merged
merged 1 commit into from
Oct 30, 2015

Conversation

mleanos
Copy link
Member

@mleanos mleanos commented Oct 29, 2015

Removes a duplicate User CRUD test for Profile Picture.

There are two reasons for this commit.

  1. Duplicate of https://github.com/meanjs/mean/blob/master/modules/users/tests/server/user.server.routes.tests.js#L833-L848
  2. This test is problematic in Windows environment.
    Related to:
    Random test success/failure on Windows ladjs/supertest#230
    On ECONNRESET, assert() calls undefined res.status which throws an error ladjs/supertest#258

The latter may be an issue with the .attach method not completely loading the file into memory before the 400 status response is sent back due to no User logged in.

Removes a duplicate User CRUD test for Profile Picture.

There are two reasons for this commit.

1) Duplicate of
https://github.com/meanjs/mean/blob/master/modules/users/tests/server/user.server.routes.tests.js#L833-L848
2) This test is problematic in Windows environment.
Related to:
ladjs/supertest#230
ladjs/supertest#258

The latter may be an issue with the `.attach` method not completely
loading the file into memory before the 400 status response is sent back
due to no User logged in.
@mleanos
Copy link
Member Author

mleanos commented Oct 29, 2015

This may seem trivial. However, I have been fighting with this issue for a few days; ever since this test was merged in. This test has been failing for me over 50% of the time.

It appears, from asking others, that this only an issue in Windows. If someone can confirm that they are experiencing an issue with this test on Windows too, that would be very helpful.

I'll keep an eye on this issue by watching the supertest & superagent libraries. But for now, this test isn't needed since the only difference between it & the duplicate is the attach method. Since this test isn't attempting to test the actual upload, we don't need to attach a file to the request.

@lirantal lirantal added this to the 0.5.0 milestone Oct 29, 2015
@lirantal lirantal self-assigned this Oct 29, 2015
@lirantal
Copy link
Member

@mleanos the test you removed is indeed less verbose also from the one that we kept so I'm generally ok, and we definitely don't need duplicate tests.

With that said:

  1. Testing the ability to upload files is important and we need a test that does that successfully
  2. Does this test work just fine in your environment? it('should be able to change profile picture if signed in'
  3. What is the problem exactly with the .attach method on Windows?

@mleanos
Copy link
Member Author

mleanos commented Oct 29, 2015

@lirantal Yes, that test does work for me. Every other test that uses .attach works for me as well.

The issue with the one I removed, is that we are sending back the 400 error response since there is no User. It appears that something is going on with the connection. When Supertest tries to assert the response Status, the response object is undefined. I'm not exactly sure why, but for some reason the connection is being reset. When I dig deeper into Supertest, i see a ECONNRESET error.

My guess is that the file isn't fully in memory when attached to the request. Then when we send the 400 response back so quickly, something is going wrong.

In the update profile picture method, you can see a 400 error returned when an upload error occurs; this works fine everytime. Bit of a strange one this is. And it doesn't always fail; somewhere around 60% of the time.

@lirantal
Copy link
Member

I see. So we can take the image upload issue in another PR but we really need to look into it. Are you already investigating it?

@mleanos
Copy link
Member Author

mleanos commented Oct 29, 2015

What image upload issue are you referring to? AFAICS, everything else is working as expected.

I think this issue is specific to supertest/superagent. But I am going to pay attention to it. It may be hinting at a bug in something we're doing, or one of the libraries.

@lirantal
Copy link
Member

I'm talking about the image upload issue that you referenced with some test that sometimes fail and sometimes not, when using the .attach method.

@mleanos
Copy link
Member Author

mleanos commented Oct 29, 2015

Ok. Got it. Yea, I'll be investigating the root cause of the issue. Our implementation is fine right now, and all the other tests work fine.

I'll just have to stay on top of the supertest/superagent issues/discussions.

@mleanos
Copy link
Member Author

mleanos commented Oct 29, 2015

@lirantal Can this be set to 0.4.2? This is causing the tests to fail for me locally. Since this is a duplicate test, I think it can just be removed now to avoid any further issues.

@lirantal lirantal modified the milestones: 0.4.2, 0.5.0 Oct 29, 2015
@codydaig
Copy link
Member

LGTM

@mleanos
Copy link
Member Author

mleanos commented Oct 30, 2015

@lirantal To follow up on my investigation into the issue with this test.. These are my findings.
ladjs/superagent#747

I'll keep on top of this issue, but it's really not relevant to us at the moment.

This PR is ready to be merged.

@lirantal
Copy link
Member

Ok, keep us posted on that issue opened in superagent

lirantal added a commit that referenced this pull request Oct 30, 2015
@lirantal lirantal merged commit 1cd909a into meanjs:master Oct 30, 2015
@mleanos
Copy link
Member Author

mleanos commented Oct 30, 2015

Will do. Thanks!

@mleanos mleanos deleted the duplicate-test-profile-picture branch October 30, 2015 05:34
@varunj90
Copy link

@mleanos , @lirantal

any resolution to your issue? I'm getting a similar issue where the res is undefined because of which im getting error noted in this ladjs/supertest#298

this is using supertest framework. any thoughts on this issue? thanks

@lirantal
Copy link
Member

@varunj90 I did not experience an actual issue but rather I think it was @mleanos said that there is some kind of issue under Windows.

@varunj90 It is probably wiser to open a new issue and put all of the information there with how to reproduce, your environment, etc.

@varunj90
Copy link

@lirantal thanks ! i've already added the details in 298 : ladjs/supertest#298

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants