Repository: lightkurve/lightkurve

Issue:
Make default `download_dir` configurable
<!-- Fill in the information below before opening an issue. -->

#### Problem description
<!-- Provide a clear and concise description of the issue. -->

Right now, when users download lightcurve / TPF files, 
1. they'd by default be saved at `~/.lightkurve-cache`, unless users override it with `download_dir` parameter in `download()` / `download_all()` call.
2. When lightcurve files are downloaded as part of `CBVCorrector`, users cannot change the location at all (there is no `download_dir`). See https://github.com/lightkurve/lightkurve/blob/2f1dda2a9efaa884c66edc2e07bed6b77e8443fc/src/lightkurve/correctors/metrics.py#L330


#### Feature Request

1. let users optionally specify the default `download_dir`
2. Bonus: For `CBVCorrector` case, also expose `download_dir` so that users can override it at per-object basis.  

Discussion:
orionlee: User Documentation changes:

`download()` (`download_all()` has similar changes):
![image](https://user-images.githubusercontent.com/250644/172491630-a7c8b567-e1b6-4fad-a41d-0ac45f606e09.png)

The new `get_cache_dir()`:
![image](https://user-images.githubusercontent.com/250644/172491713-ced039ef-8d9e-4c24-9fc2-16877475627e.png)

---

Developer Documentation changes (for running unit test locally)

![image](https://user-images.githubusercontent.com/250644/172493889-03d57cd9-d4e9-4096-86ca-f1cc34f56782.png)

orionlee: @barentsen Feel free to provide any feedback / comments.

There is still an outstanding task of supporting users in migration from `~/.lightkurve-cache` to `~/.lightkurve/cache` (issue an warning, and/or provide helper function for migration, etc.), but is largely isolated use cases and code changes that shouldn't affect the main changes in codes and docs.

orionlee: @barentsen Feedback appreciated on `~/.lightkurve-cache` migration

Originally I intended to provide some migration help: check whether `~/.lightkurve-cache` exists, and issue a warning if it does.

However, such warnings could become nuisances if users also use codes that rely on old versions of lightkurve (in particular lightkurve v1.x): `~/.lightkurve-cache` will still need to be there for such cases.

A migration by making `~/.lightkurve-cache` a symlink to the new `~/.lightkurve/cache` does not work either: the existing lightkurve codes require `~/.lightkurve-cache` to be a directory, and reject symlink (treated as a boundary condition and will use current directory instead).

To handle the migration, I think we have the following alternatives:

1. We warn users if `~/.lightkurve-cache` exists. For users who still require old versions of `lightkurve`, they have to suppress the warning themselves, or we provide a config parameter to disable the warning.
2. We only document that the migration is needed. For users who still require old versions of `lightkurve`, we suggest that they set the new `search_result_download_dir` config parameter to `~/.lightkurve-cache` (the old location) so that a single cache directory can be used.
3. Back off from moving `~/.lightkurve-cache` to `~/.lightkurve/cache`

---

Some (probably a subset of) packages that depend on `lightkurve`:
https://libraries.io/pypi/lightkurve/dependents

barentsen: @orionlee Would it make sense to name the parameter `download_dir` instead of `search_result_download_dir`?

Apart from being shorter, it will make it easier to re-use the parameter when downloading files not related to the SearchResult API (e.g. when downloading CBV files?)

I believe Astropy config files can have top-level parameters, so perhaps we could have a top-level `download_dir` parameter instead of the `search_result_download_dir` parameter inside the `[search]` section?
orionlee: @barentsen I was just thinking about renaming `search_result_download_dir` to `cache_dir` before receiving your comment. I think `cache_dir` is a bit more appropriate. In addition to the possibility of hosting CBV files, conceivably it'd serve as the basedir of persistent search result cache. 

**Planned action**: rename `search_result_download_dir` to a top level `cache_dir`.

Additionally, feedback would be appreciated for migration from `~/.lightkurve-cache` to `~/.lightkurve/cache`.
- I'm sightly leaning towards warning users if `~/.lightkurve-cache` exists, with a config option to turn off the warning (for cases that users are stuck with `~/.lightkurve-cache` because they still use , say, Lightkurve v1.x ).
- I also think we should give a heads-up to the users of a upcoming migration, e.g., an announcement on the Discussions board. I'm unclear on the reach of the Discussions board though.

orionlee: - I've renamed `search_result_download_dir` to `cache_dir` under `[config]` section. See the end for the reasoning for moving it to `[config]` section rather than keeping it at top level.
- all default cache dir logic is now centralized in `config.get_cache_dir()` function.

---
Reason for keeping `cache_dir` in `[config]` section: 
- syntactically it can be kept as a top level config parameter. I feel it can be a bit error prone.

Scenario. If an user has an existing config:
```cfg
[search]
search_result_display_extra_columns = proposal_id,
```
The user wants to config `cache_dir` and append the config to the end.
```cfg
[search]
search_result_display_extra_columns = proposal_id,

cache_dir = /my_research/cache
```
The `cache_dir` *would not* be used, as it is seen as under `[search]` section.

While the mistake could be avoided by putting `cache_dir` ahead of the existing parameters, I think it's better to keep `cahce_dir` under `[config]` section, so that mistakes similar to the above scenario could be avoided. It also maps nicely to the API semantics, as it does affect the value of `config.get_cache_dir()` after the refactoring in the latest commit.

orionlee: The documentation is now centralized in `config.get_cache_dir()`. Search's `download()` / `download_all()` now reference `config.get_cache_dir()`

![image](https://user-images.githubusercontent.com/250644/173670996-ccd699cb-c2fc-4686-9ad2-60f13595ca2b.png)

barentsen: > I think `cache_dir` is a bit more appropriate.

Yes, `cache_dir` sounds good to me!

> We warn users if `~/.lightkurve-cache` exists. 

I think this is a good solution, in particular if we ensure that (i) the warning is only raised once (upon `import lightkurve`!?) and (ii) we include a helpful suggestion on the action the user should take (deleting the directory?)

> I also think we should give a heads-up to the users of a upcoming migration, e.g., an announcement on the Discussions board.

While I don't expect an announcement in the Discussion board to reach a lot of users, it does seem like the perfect place to ensure that users who Google the warning message will easily find a webpage with an explanation. TL;DR I think it's a good idea if we copy/paste the full warning message in the thread!

> I've renamed `search_result_download_dir` to `cache_dir` under `[config]` section.

I agree your reasoning!  Perhaps the `cache_dir` parameter could be defined as a parameter of the `[io]` module, but I do think it's better to have it in the same place as the `config_dir`, i.e. I think `[config]` is the way to go.

:+1::+1::+1:
orionlee: A warning is now issued when importing `lightkurve`:

![image](https://user-images.githubusercontent.com/250644/173965057-bd084aa0-c1bd-425b-ac53-de0b0c69202a.png)

I considered to issue the warning in `download()` / `download_all()` call, but feel the warning might become excessive, so I have the check in the package init instead.


The documentation that considers migration suggestion referenced by the warning:

![image](https://user-images.githubusercontent.com/250644/173965405-dac892c3-f84b-4316-b67c-d6d90e83ac26.png)

barentsen: I like the `UserWarning`!  Would it make sense to raise it only when the `SearchResult.download()` or `SearchResult.download_all()` method is called, to avoid confusing users who may not be downloading data products?
orionlee: Yeah, raising the `UserWarning` in `SearchResult.download()` / `.download_all()` would be a more natural place, and work better for users who don't use `lightkurve` to download files as you suggested.

I ended up doing it in `import lightkurve` because if some users choose to ignore the warning for whatever reasons, repeated warnings when calling `download()` could be a bit annoying. 

I understand in theory, a warning should only show up once only by default, but somehow it often is not the case in practice in my experience (I never understand the reasons). On the flip side, one could argue that users really should not ignore the warning anyway, especially they are given a way to suppress it.

I was on the fence between raising the warning at `download()` vs at `import lightkurve` to begin with. If you feel also feel `download()` is better, then let's go with it.



barentsen: > I ended up doing it in import lightkurve because if some users choose to ignore the warning for whatever reasons, repeated warnings when calling download() could be a bit annoying.

That would be super annoying.  Fortunately, I believe warnings issued via `warnings.warn()` only show up once by default (unless configured otherwise, e.g. via `python -W all`), whereas warnings issued via `log.warning()` more commonly display multiple times.  We'd want to use `warnings.warn()` in this case for reasons summarized in https://github.com/lightkurve/lightkurve/issues/691#issuecomment-586016365
orionlee: GRA™️ - :rofl:

Anyway, so let's move the warning raising from `import lightkurve` to `download() / donwload_all()` then.

orionlee: @barentsen I've moved the raising of default cache dir migration warning so that it is triggered only when `download()` / `download_all()` is called. 

However, when testing in my own environments (command line and Jupyter notebooks), the warning is repeatedly emitted to the display (see the sample script at the end for an example).

However, in a simpler isolated script, the `UserWarning` is indeed emitted to the display once only. 

I don't know what's missing. I did verify it is not due to warning filters configuration changes after importing `lightkurve`.


The lightkurve script that shows repeated `UserWarning` with this PR.
```python
import warnings
import pprint
import lightkurve as lk

sr = lk.search_lightcurve('tic77031414', sector=2, exptime=120, author='spoc')
print(sr)

lc = sr[0].download()
print("Download #1 ", len(lc))

lc = sr[0].download()
print("Download #2 ", len(lc))

lc = sr[0].download()
print("Download #3 ", len(lc))

lcc = sr.download_all()
print("Download_all #1 ", len(lcc))


print("Warning filters config")
pp = pprint.PrettyPrinter(indent=4)
pp.pprint(warnings.filters)
```


Isolated script that shows `UserWarning` only once, even with `import lightkurve`:
```
import warnings
import pprint

# see if the act of importing lightkurve will lead to UserWarning being emitted repeatedly
# (the answer is no)
import lightkurve as lk

def try_warning():
    warnings.warn("Some warning", category=UserWarning)


try_warning()
try_warning()
try_warning()

print("Warning filters config")
pp = pprint.PrettyPrinter(indent=4)
pp.pprint(warnings.filters)
```

orionlee: I can't make `UserWarning` show up once only.

I also tried explicitly add a warning filter with `default` action in `config/__init__.py`. It did not have any effect.

```python
warnings.filterwarnings(
    "default",
    message=".*default Lightkurve cache directory has been moved.*",
    category=UserWarning,
    module=__name__)
```

- I tried action `once`. It did not work either.
- When I specified action `ignore`, the warning is properly suppressed.
- So the filter is catching the warning, but somehow `default` or `once` did not work.



orionlee: Let's go with emitting warnings once upon `import lightkurve` then.

We could amend the warning message to state that it is related to `donwload()`, so that the users would know what the warning is related to download even when it shows up after `import lightkurve`:

Amendment in **_bold italic_**:
> UserWarning: The default Lightkurve cache directory, **_used by download(), etc.,_** has been moved to [new-dir]. Please move all the files in the legacy directory [legacy-dir] to the new location and remove the legacy directory. Refer to https://docs.lightkurve.org/reference/config.html#default-cache-directory-migration for more information.


Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 6b1c50ab..4f6f3b3a 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -1,3 +1,12 @@
+2.4.0 (unreleased)
+==================
+
+- Added the ability to configure the default cache directory used by
+  ``SearchResult.download()`` / ``SearchResult.download_all()``. [#1214]
+- Moved the default cache directory from ``$HOME/.lightkurve-cache``
+  to ``$HOME/.lightkurve/cache``. [#1214]
+
+
 2.3.0 (2022-07-07)
 ==================
 
diff --git a/docs/source/reference/config.rst b/docs/source/reference/config.rst
index 129538d0..b9aaa06e 100644
--- a/docs/source/reference/config.rst
+++ b/docs/source/reference/config.rst
@@ -22,4 +22,38 @@ Access configuration values
   :toctree: api/
 
   conf
+  config.get_cache_dir
   config.get_config_dir
+
+
+Default Cache Directory Migration
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Starting from ``Lightkurve`` version ``2.4.0``, the default cache directory is
+at ``~/.lightkurve/cache`` . The data files cached at the legacy location,
+``~/.lightkurve-cache``, will not be used.
+
+A warning is issued if the legacy ``~/.lightkurve-cache`` directory still exists.
+
+Migration suggestions for handling various scenarios:
+
+* To use the existing data files cached, move all the contents under ``~/.lightkurve-cache``
+  to ``~/.lightkurve/cache``, and remove ``~/.lightkurve-cache`` directory itself.
+
+* If you need to use older version of ``Lightkurve``, e.g., because of the requirements
+  of other packages / applications, you can:
+
+  #. Keep the cache at the legacy location ``~/.lightkurve-cache``
+  #. Instruct current ``Lightkurve`` to use the legacy location. In the user's ``lightkurve.cfg``, add:
+
+      [config]
+
+      cache_dir = /<your-home-directory>/.lightkurve-cache
+
+  #. The warning will no long appear once a custom ```cache_dir``` is specified.
+
+* To suppress the warning for any reason, you can set ``warn_legacy_cache_dir`` in the user's ``lightkurve.cfg``.
+
+    [config]
+
+    warn_legacy_cache_dir = False
diff --git a/src/lightkurve/__init__.py b/src/lightkurve/__init__.py
index ec4c0bd7..b10773f7 100644
--- a/src/lightkurve/__init__.py
+++ b/src/lightkurve/__init__.py
@@ -57,6 +57,19 @@ class Conf(_config.ConfigNamespace):
 
     Refer to `Astropy documentation <https://docs.astropy.org/en/stable/config/index.html#accessing-values>`_
     for usage.
+
+    The attributes listed below are the available configuration parameters.
+
+    Attributes
+    ----------
+    search_result_display_extra_columns
+        List of extra columns to be included when displaying a SearchResult object.
+
+    cache_dir
+        Default cache directory for data files downloaded, etc. Defaults to ``~/.lightkurve/cache`` if not specified.
+
+    warn_legacy_cache_dir
+        If set to True, issue warning if the legacy default cache directory exists. Default is True.
     """
     # Note: when using list or string_list datatype,
     # the behavior of astropy's parsing of the config file value:
@@ -71,6 +84,19 @@ class Conf(_config.ConfigNamespace):
         module="lightkurve.search"
     )
 
+    cache_dir = _config.ConfigItem(
+        None,
+        "Default cache directory for data files downloaded, etc.",
+        cfgtype="string",
+        module="lightkurve.config"
+    )
+
+    warn_legacy_cache_dir = _config.ConfigItem(
+        True,
+        "If set to True, issue warning if the legacy default cache directory exists.",
+        cfgtype="boolean",
+        module="lightkurve.config"
+    )
 
 conf = Conf()
 
@@ -87,3 +113,6 @@ from .convenience import *
 from .collections import *
 from .io import *
 from .search import *
+
+from . import config
+config.warn_if_default_cache_dir_migration_needed()
diff --git a/src/lightkurve/config/__init__.py b/src/lightkurve/config/__init__.py
index 7a8281d5..741b246b 100644
--- a/src/lightkurve/config/__init__.py
+++ b/src/lightkurve/config/__init__.py
@@ -1,3 +1,6 @@
+import os
+import warnings
+
 import astropy.config as astropyconfig
 
 
@@ -30,3 +33,93 @@ def get_config_dir():
     """
     return astropyconfig.get_config_dir(ROOTNAME)
 
+
+def get_cache_dir():
+    """
+    Determines the default Lightkurve cache directory name and creates the
+    directory if it doesn't exist. If the directory cannot be access or created,
+    then it returns the current directory (``"."``).
+
+    This directory is typically ``$HOME/.lightkurve/cache``, but if the
+    XDG_CACHE_HOME environment variable is set and the
+    ``$XDG_CACHE_HOME/lightkurve`` directory exists, it will be that directory.
+    If neither exists, the former will be created and symlinked to the latter.
+
+    The value can be also configured via ``cache_dir`` configuration parameter.
+
+    Returns
+    -------
+    cachedir : str
+        The absolute path to the cache directory.
+
+    Examples
+    --------
+    To configure "/my_research/data" as the `cache_dir`, users can set it:
+
+    1. in the user's ``lightkurve.cfg`` file::
+
+        [config]
+        cache_dir = /my_research/data
+
+    2. at run time::
+
+        import lightkurve as lk
+        lk.conf.cache_dir = '/my_research/data'
+
+    See :ref:`configuration <api.config>` for more information.
+    """
+    from .. import conf
+
+    cache_dir = conf.cache_dir
+    if cache_dir is None or cache_dir == "":
+        cache_dir = astropyconfig.get_cache_dir(ROOTNAME)
+    cache_dir = _ensure_cache_dir_exists(cache_dir)
+    cache_dir = os.path.abspath(cache_dir)
+
+    return cache_dir
+
+
+def _ensure_cache_dir_exists(cache_dir):
+    if os.path.isdir(cache_dir):
+        return cache_dir
+    else:
+        # if it doesn't exist, make a new cache directory
+        try:
+            os.mkdir(cache_dir)
+        # user current dir if OS error occurs
+        except OSError:
+            warnings.warn(
+                "Warning: unable to create {} as cache dir "
+                " (for downloading MAST files, etc.). Use the current "
+                "working directory instead.".format(cache_dir)
+            )
+            cache_dir = "."
+        return cache_dir
+
+
+def warn_if_default_cache_dir_migration_needed():
+    from .. import conf
+
+    if not conf.warn_legacy_cache_dir:
+        return
+
+    cache_dir = conf.cache_dir
+    if not(cache_dir is None or cache_dir == ""):
+        # If an user has specified a custom cache dir, the check won't be performed.
+        # Not only is the check somewhat irrelevant, the behavior is also required
+        # to support the case that the user configures the legacy `~/.lightkurve-cache`
+        # as the cache dir (e.g., to support running other apps/packages that require
+        # older lightkurve, especially lightkurve v1.x.)
+        return
+
+    # migration check done only if default is used
+    old_cache_dir = os.path.join(os.path.expanduser("~"), ".lightkurve-cache")
+    new_cache_dir = os.path.join(os.path.expanduser("~"), ".lightkurve", "cache")
+    if os.path.isdir(old_cache_dir):
+        warnings.warn(
+            f"The default Lightkurve cache directory, used by download(), etc., has been moved to {new_cache_dir}. "
+            f"Please move all the files in the legacy directory {old_cache_dir} to the new location "
+            f"and remove the legacy directory. "
+            f"Refer to https://docs.lightkurve.org/reference/config.html#default-cache-directory-migration "
+            f"for more information."
+        )
diff --git a/src/lightkurve/search.py b/src/lightkurve/search.py
index 92917431..fa854fa9 100644
--- a/src/lightkurve/search.py
+++ b/src/lightkurve/search.py
@@ -21,6 +21,7 @@ from .collections import TargetPixelFileCollection, LightCurveCollection
 from .utils import LightkurveError, suppress_stdout, LightkurveWarning, LightkurveDeprecationWarning
 from .io import read
 from . import conf
+from . import config
 from . import PACKAGEDIR
 
 log = logging.getLogger(__name__)
@@ -376,7 +377,10 @@ class SearchResult(object):
             See the :class:`KeplerQualityFlags <lightkurve.utils.KeplerQualityFlags>` or :class:`TessQualityFlags <lightkurve.utils.TessQualityFlags>` class for details on the bitmasks.
         download_dir : str, optional
             Location where the data files will be stored.
-            Defaults to "~/.lightkurve-cache" if `None` is passed.
+            If `None` is passed, the value from `cache_dir` configuration parameter is used,
+            with "~/.lightkurve/cache" as the default.
+
+            See `~lightkurve.config.get_cache_dir()` for details.
         cutout_size : int, float or tuple, optional
             Side length of cutout in pixels. Tuples should have dimensions (y, x).
             Default size is (5, 5)
@@ -397,6 +401,7 @@ class SearchResult(object):
             If the TESSCut service times out (i.e. returns HTTP status 504).
         SearchError
             If any other error occurs.
+
         """
         if len(self.table) == 0:
             warnings.warn(
@@ -447,7 +452,10 @@ class SearchResult(object):
             See the :class:`KeplerQualityFlags <lightkurve.utils.KeplerQualityFlags>` or :class:`TessQualityFlags <lightkurve.utils.TessQualityFlags>` class for details on the bitmasks.
         download_dir : str, optional
             Location where the data files will be stored.
-            Defaults to "~/.lightkurve-cache" if `None` is passed.
+            If `None` is passed, the value from `cache_dir` configuration parameter is used,
+            with "~/.lightkurve/cache" as the default.
+
+            See `~lightkurve.config.get_cache_dir()` for details.
         cutout_size : int, float or tuple, optional
             Side length of cutout in pixels. Tuples should have dimensions (y, x).
             Default size is (5, 5)
@@ -495,34 +503,7 @@ class SearchResult(object):
             return LightCurveCollection(products)
 
     def _default_download_dir(self):
-        """Returns the default path to the directory where files will be downloaded.
-
-        By default, this method will return "~/.lightkurve-cache" and create
-        this directory if it does not exist.  If the directory cannot be
-        access or created, then it returns the local directory (".").
-
-        Returns
-        -------
-        download_dir : str
-            Path to location of `mastDownload` folder where data downloaded from MAST are stored
-        """
-        download_dir = os.path.join(os.path.expanduser("~"), ".lightkurve-cache")
-        if os.path.isdir(download_dir):
-            return download_dir
-        else:
-            # if it doesn't exist, make a new cache directory
-            try:
-                os.mkdir(download_dir)
-            # downloads locally if OS error occurs
-            except OSError:
-                log.warning(
-                    "Warning: unable to create {}. "
-                    "Downloading MAST files to the current "
-                    "working directory instead.".format(download_dir)
-                )
-                download_dir = "."
-
-        return download_dir
+        return config.get_cache_dir()
 
     def _fetch_tesscut_path(self, target, sector, download_dir, cutout_size):
         """Downloads TESS FFI cutout and returns path to local file.


Test patch:
diff --git a/docs/source/about/testing.rst b/docs/source/about/testing.rst
index c70b662d..25c78841 100644
--- a/docs/source/about/testing.rst
+++ b/docs/source/about/testing.rst
@@ -46,6 +46,17 @@ Running some of our tests requires external data, e.g. some require data to be d
 
     poetry run pytest test_targetpixelfile.py --remote-data
 
+.. note::
+    To speed up the tests, you can instruct the tests use a specific directory for data file cache, so that when the tests are re-run,
+    data files in the local directory will be used, instead of being downloaded over the network.
+
+    Specify the data file cache directory via `XDG_CACHE_HOME` environment variable. For example,
+
+    .. code-block:: bash
+
+        $ XDG_CACHE_HOME=/my_dev/lk-test-cache poetry run pytest test_targetpixelfile.py --remote-data
+
+    The data files will be cached at `/my_dev/lk-test-cache/lightkurve` in the above example.
 
 
 My tests passed, but I got warning messages
diff --git a/tests/conftest.py b/tests/conftest.py
index 9de498e6..48400b98 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -42,8 +42,20 @@ def pytest_collection_modifyitems(config, items):
         if 'memtest' in item.keywords:
             item.add_marker(skip_memtest)
 
-# Make sure we use temporary directories for the config
+# Make sure we use temporary directories for the config and cache
 # so that the tests are insensitive to local configuration.
 
 os.environ['XDG_CONFIG_HOME'] = tempfile.mkdtemp('lightkurve_config')
-os.mkdir(os.path.join(os.environ['XDG_CONFIG_HOME'], 'lightkurve'))
\ No newline at end of file
+os.mkdir(os.path.join(os.environ['XDG_CONFIG_HOME'], 'lightkurve'))
+
+# Let users optionally specify XDG_CACHE_HOME for a test run
+# use case: in a local dev env, an user might want to reuse an existing dir for cache,
+# so as to speed up remote-data tests
+if os.environ.get('XDG_CACHE_HOME', '') == '':
+    os.environ['XDG_CACHE_HOME'] = tempfile.mkdtemp('lightkurve_cache')
+else:
+    print(f"lightkurve conftest: Use user-specified XDG_CACHE_HOME: {os.environ['XDG_CACHE_HOME']}")
+
+_cache_dir = os.path.join(os.environ['XDG_CACHE_HOME'], 'lightkurve')
+if not os.path.isdir(_cache_dir):
+    os.mkdir(_cache_dir)
diff --git a/tests/test_conf.py b/tests/test_conf.py
index 495960b9..65c03f20 100644
--- a/tests/test_conf.py
+++ b/tests/test_conf.py
@@ -1,10 +1,13 @@
 from astropy.utils.data import get_pkg_data_filename
 
+import os
 from pathlib import Path
 import shutil
+import tempfile
 
 import lightkurve as lk
 
+
 def test_read_conf_from_file():
     """Sanity test to ensure lightkurve per-user config is in the expected location."""
 
@@ -15,7 +18,7 @@ def test_read_conf_from_file():
         use_custom_config_file("data/lightkurve_sr_cols_added.cfg")
         assert ['proposal_id'] == lk.conf.search_result_display_extra_columns
     finally:
-        # cleanup: remvoe the custom config file and its effect
+        # cleanup: remove the custom config file and its effect
         remove_custom_config()
 
 
@@ -31,3 +34,29 @@ def remove_custom_config():
     cfg_dest_path = Path(lk.config.get_config_dir(), 'lightkurve.cfg')
     cfg_dest_path.unlink()
     lk.conf.reload()
+
+
+def test_get_cache_dir():
+    # Sanity test default download dir
+    # We can't meaningful assert the location for typical cases.
+    # Because in test environment, it is overriden by XDG_CACHE_HOME env var
+    # (typically to some temp location)
+    actual_dir = lk.config.get_cache_dir()
+    assert os.path.isdir(actual_dir)
+
+    # Test customized default download dir
+    with tempfile.TemporaryDirectory() as expected_base:
+        try:
+            # I want to test that the impl would create a dir if not there
+            expected_dir = os.path.join(expected_base, "some_subdir")
+            lk.conf.cache_dir = expected_dir
+            actual_dir = lk.config.get_cache_dir()
+            assert expected_dir == actual_dir
+            assert os.path.isdir(actual_dir)
+
+            # repeated calls would work
+            # (e.g., it won't raise errors in attempting to mkdir for an existing dir)
+            actual_dir = lk.config.get_cache_dir()
+            assert expected_dir == actual_dir
+        finally:
+            lk.conf.cache_dir = None