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

Added more methods with unittests #10

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

ESchae
Copy link

@ESchae ESchae commented Jan 4, 2018

New methods in class _Survey:

  • delete_survey
  • export_responses
  • import_survey
  • activate_survey

New methods in class _Token:

  • remind_participants

All with unittests that cover the most important functionality.

ESchae and others added 30 commits January 3, 2018 16:40
…uccess and test_list_participants_return_type_success
added export_responses and basic unittest
@vectorson
Copy link

Nice work! Happy to see export_responses and remind_participants being implemented.

I noticed that your tests for remind_participants are not asserting that the number of mails captured by the CapturingAiosmtpdServer are correct. After reading your issue (#9), I'm guessing you're having problems with the aiosmtpd server. Have you had the chance to troubleshoot this any further? I'm assuming you have configured the email settings of your LS installation according to the readme. Correct?

@ESchae
Copy link
Author

ESchae commented Jan 16, 2018

Actually I read over the configurations in the readme so thank you very much for the hint. Unfortunately changing the settings did not solve the issue. I even tried

  • SMTP username, password: blank --> ""
  • SMTP username, password: blank --> " "
  • SMTP username, password: blank --> "blank"

But none of these worked. Maybe the problem arises because I am currently using LimeSurvey Version 2.62.0+170124. The installation will be updated next week, so I will try again then.

A general question: For my usage of LimeSurvey I need different email settings than those specified in the readme for testing. How do you deal with this? Do you have an extra LS installation + admin account only for testing? Do you simply have the same settings no matter whether you are running tests or working with LimeSurvey? In my case I would have to change the settings always before and after running tests. Of course I could do this automatically with the API, but I wonder whether I misunderstood something or whether there would be a more elegant option.

@vectorson
Copy link

It sounds like your problem is that you're running your tests on your local machine against a remote LS server (via tests/config.ini) that can't connect to the CAS (CapturingAiosmtpdServer) used in the tests. Don't forget that the CAS starts up a temporary SMTP server at port 10025 on the machine running the tests. The CAS is used to verify that the mails sent by LS actually gets sent to the configured SMTP server. Therefore, the simplest way to run those tests is to have an LS installation with SMTP host set to localhost on the machine running the tests.

Developing and running unit tests against a production environment is not a very good idea, so I recommend you to install LS on a VM on your machine and run the tests on it.

@lindsay-stevens
Copy link
Owner

Hi @ESchae - thanks very much for this PR. As discussed above I think the testing situation could be better. I was thinking of poaching LimeSurvey's travis config to spin up a test instance to work with - ideally converting it to docker containers so it's easier to pre-check locally. I thought I'd mention it in case you or @vectorson already have something like that, or are interested in working it out. Thanks again!

@ESchae
Copy link
Author

ESchae commented Jan 20, 2018

There already exists a limesurvey docker. I did not have the time to test it so far, but maybe this could be used to make testing easier.

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.

3 participants